Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

backport: bitcoin#11909, #20586, #21090, #21500, #21562, #22232, #22378, #22688, bitcoin-core/gui#381, #390 #6360

Merged
merged 9 commits into from
Oct 25, 2024
Merged

backport: bitcoin#11909, #20586, #21090, #21500, #21562, #22232, #22378, #22688, bitcoin-core/gui#381, #390 #6360

merged 9 commits into from
Oct 25, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Oct 25, 2024

What was done?

Regular batch of backports from Bitcoin v23

How Has This Been Tested?

Run unit and functional tests

Breaking Changes

N/A

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

@knst
Merge bitcoin#22232: refactor: Pass interpreter flags as uint32_t ins…
e3c69da 
…tead of signed int

BACKPORT NOTE:
Prior work is done here: dashpay@c338cd6#diff-060e8fd790fc1c3e18c64327a7395bb5b2d6d57db9792cc666bd8d7354a40c0b
Missing changes in tx_verify.h included in this PR, but changes in
src/test/sigopcount_tests.cpp and src/test/transaction_tests.cpp are
irrelevant because they are segwit-related

fa621ed refactor: Pass script verify flags as uint32_t (MarcoFalke)

Pull request description:

  The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.

  Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.

  Fixes bitcoin#22233

ACKs for top commit:
  theStack:
    Concept and code review ACK fa621ed
  kristapsk:
    ACK fa621ed
  jonatack:
    ACK fa621ed

Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
UdjinM6
UdjinM6 reviewed Oct 25, 2024
View reviewed changes
test/functional/mining_basic.py Outdated Show resolved Hide resolved
test/functional/test_framework/messages.py Outdated Show resolved Hide resolved
contrib/builder-keys/README.md Outdated Show resolved Hide resolved
MarcoFalke and others added 8 commits October 25, 2024 20:51
@knst
Merge bitcoin#21562: [net processing] Various tidying up of PeerManag…
42a0cf0 
…erImpl ctor

fde1bf4 [net processing] Default initialize m_recent_confirmed_transactions (John Newbery)
37dcd12 scripted-diff: Rename recentRejects (John Newbery)
cd9902a [net processing] Default initialize recentRejects (John Newbery)
a28bfd1 [net processing] Default initialize m_stale_tip_check_time (John Newbery)
9190b01 [net processing] Add Orphanage empty consistency check (John Newbery)

Pull request description:

  - Use default initialization of PeerManagerImpl members where possible
  - Remove unique_ptr indirection where it's not needed

ACKs for top commit:
  MarcoFalke:
    ACK fde1bf4 👞
  theStack:
    re-ACK fde1bf4

Tree-SHA512: 7ddedcc972df8e933e1fbe5c88b8ea17df89e1e58fc769518512c5540e49dc8eddb3f47e78d1329a6fc5644d2c1d11c981f681fd633f5218bfa4b3e6a86f3d7b
@knst
Merge bitcoin#22378: test: remove confusing MAX_BLOCK_BASE_SIZE
5a803ae 
607076d test: remove confusing `MAX_BLOCK_BASE_SIZE` (Sebastian Falbesoner)
4af97c7 test: introduce `get_weight()` helper for CBlock (Sebastian Falbesoner)
a084ebe test: introduce `get_weight()` helper for CTransaction (Sebastian Falbesoner)

Pull request description:

  This is a very late follow-up PR to bitcoin#10618, which removed the constant `MAX_BLOCK_BASE_SIZE` from the core implementation about four years ago (see also bitcoin#10608 in why it was considered confusing and superfluous).
  Since there is also no point in still keeping it in the functional test framework, the PR switches to weight-based accounting on the relevant test code parts and use `MAX_BLOCK_WEIGHT` instead for the block limit checks. To prepare that, the first two commits introduce `get_weight()` helpers for the classes CTransaction and CBlock, respectively.

ACKs for top commit:
  MarcoFalke:
    review ACK 607076d 🚴

Tree-SHA512: d59aa0b6b3dfd0a849b8063e66de275d252f705f99e25cd3bf6daec028b47d946777ee5b42a060f5283cb18e917ac073119c2c0e11bbc21211f69ef0a6ed335a
@meshcollider @knst
Merge bitcoin#21500: wallet, rpc: add an option to list private descr…
1ff42b4 
…iptors

bb822a7 wallet, rpc: add listdescriptors private option (S3RK)

Pull request description:

  Rationale: make it possible to backup your wallet with `listdescriptors` command

  * The default behaviour is still to show public version
  * For private version only the root xprv is returned

  Example use-case:
  ```
  > bitcoin-cli -regtest -named createwallet wallet_name=old descriptors=true
  > bitcoin-cli -regtest -rpcwallet=old listdescriptors true | jq '.descriptors' > descriptors.txt

  > bitcoin-cli -regtest -named createwallet wallet_name=new descriptors=true blank=true
  > bitcoin-cli -regtest -rpcwallet=new importdescriptors "$(cat descriptors.txt)"
  ```

  In case of watch-only wallet without private keys there will be following output:
  ```
  error code: -4
  error message:
  Can't get descriptor string.
  ```

ACKs for top commit:
  achow101:
    re-ACK bb822a7
  Rspigler:
    tACK bb822a7
  jonatack:
    ACK bb822a7 per `git diff 2854ddc bb822a7`
  prayank23:
    tACK bitcoin@bb822a7
  meshcollider:
    Code review ACK bb822a7

Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
@laanwj @knst
Merge bitcoin#11909: contrib: Replace developer keys with list of pgp…
62b5358 
… fingerprints

fabb72b contrib: Remove xpired 522739F6 key (MarcoFalke)
faeab66 contrib: Replace developer keys with list of pgp fingerprints (MarcoFalke)

Pull request description:

  Having to host a copy of the keys in this repo was a common source of discussion and distraction, caused by problems such as:

  * Outdated keys. Unclear whether and when to replace by fresh copies.
  * Unclear when to add a key of a new developer or Gitian builder.

  The problems are solved by
  * Having no keys but only the fingerprints
  * Adding a rule of thumb, when to add a new key

  <strike>Moving the keys to a different repo solves none of these issues, but since the keys are not bound to releases or git branches of Bitcoin Core, they should live somewhere else.

  Obviously, all keys are hosted and distributed on key servers, but were added to the repo solely for convenience and redundancy.

  Moving the mirror of those keys to a different repo makes it less distracting to update them -- let's say -- prior to every major release.

  I updated our `doc/release-process.md` to reflect the new location.

  DEPENDS_ON bitcoin-core/gitian.sigs#621
  </strike>

Tree-SHA512: c00795a07603190e26dc4526f6ce11e492fb048dc7ef54b38f859b77dcde25f58ec4449f5cf3f85a5e9c2dd2743bde53f7ff03c8eccf0d75d51784a6b164e47d
@fanquake @knst
Merge bitcoin#22688: contrib: use keys.openpgp.org to retrieve buil…
40a8b92 
…der keys

4c43b7d contrib: use hkps://keys.openpgp.org to retrieve builder keys (fanquake)

Pull request description:

  `hkps://hkps.pool.sks-keyservers.net` is essentially no-longer functional,
  and a number of distributions and GPG tools have since switched to using
  the `keys.openpgp.org` key server as their default.

  See this Debian patch for additional context:
  https://salsa.debian.org/debian/gnupg2/-/blob/debian/main/debian/patches/Use-hkps-keys.openpgp.org-as-the-default-keyserver.patch

  Switch to using keys.openpgp.org in the CI as well.

ACKs for top commit:
  MarcoFalke:
    cr ACK 4c43b7d
  Zero-1729:
    ACK 4c43b7d

Tree-SHA512: e6c72b67778b76f81c659eee0e4195fea9e579587c64921affd35b9d46a077d4e8754b7fb85ca90a9a4bbc5cd5a47b0c6e4c9dbf9a335418a12f774d665e5a19
@laanwj @knst
Merge bitcoin-core/gui#381: refactor: Make BitcoinCore class reusable
7df9788 
8169fc4 qt, refactor: Fix code styling of moved InitExecutor class (Hennadii Stepanov)
c82165a qt, refactor: Move InitExecutor class into its own module (Hennadii Stepanov)
dbcf56b scripted-diff: Rename BitcoinCore class to InitExecutor (Hennadii Stepanov)
19a1d00 qt: Add BitcoinCore::m_thread member (Hennadii Stepanov)

Pull request description:

  This PR makes the `BitcoinCore` class reusable, i.e., it can be used by the widget-based GUI or by the [QML-based](https://github.com/bitcoin-core/gui-qml/tree/main/src/qml) one, and it makes the divergence between these two repos minimal.

  The small benefit to the current branch is more structured code.

  Actually, this PR is ported from bitcoin-core/gui-qml#10.
  The example of the re-using of the `BitcoinCore` class is bitcoin-core/gui-qml#11.

ACKs for top commit:
  laanwj:
    ACK 8169fc4
  ryanofsky:
    Code review ACK 8169fc4. Only change is switching from `m_executor` from pointer to optional type (thanks for update!)

Tree-SHA512: a0552c32d26d9acf42921eb12bcdf68f02d52f7183c688c43257b1a58679f64e45f193ee2d316850c7f0f516561e17abe989fe545bfa05e158ad3f4c66d19bca
@hebasto @knst
Merge bitcoin-core/gui#390: Add SubFeeFromAmount to options
368a6ef 
62b125f qt, refactor: Fix indentation (Prateek Sancheti)
ad28b66 qt: Add SubFeeFromAmount option (Prateek Sancheti)

Pull request description:

  This PR adds **_SubFeeFromAmount_** option which lets the user select their preferred setting of whether fee for a transaction is to be subtracted from the amount or not for future transactions. The setting chosen by the user is remembered even when the GUI mode is turned off.

  **_Functionality and Usage:_**

  - Go to `Settings > Options > Wallet` on _Windows/Linux_ or `bitcoin-qt > Preferences > Wallet` on _macOS_.
  - The checkbox **Subtract Fee From Amount** corresponds to the added option **SubFeeFromAmount**.
  - The preferred setting intended to be the default for all future send transactions should be selected by the user.
  - Click on **OK**.
  - Go to the **Send** tab in the wallet.
  - You shall notice, any new Send transaction created will have the preferred setting as chosen by the user.<br> (Try clicking on Add recipient or even restarting the Node in GUI)

  Attaching ScreenRecordings to explain the added feature.

  > Master.mov: Master Branch

  https://user-images.githubusercontent.com/54016434/127763378-be91837d-d0ab-4ae5-87c0-d303fa70a336.mov

  > PR.mov: PullRequest

  https://user-images.githubusercontent.com/54016434/127763404-05b834c1-4082-4fbd-9b05-1528ac898a21.mov

  Close dashpay#386

ACKs for top commit:
  Talkless:
    tACK 62b125f, tested on Debian Sid with 5.15.2 and it works as described.
  hebasto:
    re-ACK 62b125f, only removed the unused `SubFeeFromAmountChanged` signal since my [previous](bitcoin-core/gui#390 (review)) review.
  meshcollider:
    utACK 62b125f

Tree-SHA512: 932ca89ae578a1e1c426561400d87cf005c231944feaf0f662ff8d88f32bdd65a927a090ea41510a15f8ec0ebcd5529672e9917720eb5ea85f413f081e45d5bb
@laanwj @knst
Merge bitcoin#20586: Fix Windows build with --enable-werror
750447e 
BACKPORT NOTE:
Missing changes in src/fs.cpp are removed in bitcoin#20744 which is already backported

b367745 ci: Make Cirrus CI Windows build with --enable-werror (Hennadii Stepanov)
c713bb2 Fix Windows build with --enable-werror on Ubuntu Focal (Hennadii Stepanov)

Pull request description:

  This PR makes possible to cross-compile Windows build with `--enable-werror --enable-suppress-external-warnings`.
  Some problems are fixed, others are silenced.

  Also `--enable-werror` is enabled for Cirrus CI Windows build (the last one on Cirrus CI without `--enable-werror`).

ACKs for top commit:
  practicalswift:
    cr ACK b367745: patch looks correct
  laanwj:
    Code review ACK b367745
  vasild:
    ACK b367745
  jarolrod:
    ACK b367745

Tree-SHA512: 64f5c99b7dad4c0efce80cd45d7074f275bd8411235dc9e0841287bdab64b812c6f8f9d632c35531d0b8210148531f53aaaac77be7699b29d2d6aaae304dbee0
@knst knst requested a review from UdjinM6 October 25, 2024 14:12
UdjinM6
UdjinM6 approved these changes Oct 25, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 750447e

PastaPastaPasta
PastaPastaPasta approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 750447e

@PastaPastaPasta PastaPastaPasta merged commit a0ab06f into dashpay:develop Oct 25, 2024
35 of 37 checks passed
@knst knst deleted the bp-v23-p2 branch October 25, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@knst @UdjinM6 @PastaPastaPasta @meshcollider @laanwj @fanquake @hebasto