Backports 0.18 pr20#4675
Merged
Merged
Conversation
Munkybooty
force-pushed
the
backports-0.18-pr20
branch
from
841aba6 to
c7fdafd
Compare
Member
There was a problem hiding this comment.
13968: I'm not sure we want just this. Why is this just this?
Author
There was a problem hiding this comment.
iirc, the remainder of the pr deals entirely with rbf
Comment on lines
1288
to
1296
Member
There was a problem hiding this comment.
This is missing some previous back port to allow us to use success and reject_reason
EDIT: need to backport (at least part of) 11817
Author
There was a problem hiding this comment.
Oh shoot, I'm seeing this now. Will get on it
Author
There was a problem hiding this comment.
Are you sure its 11817, it only touches feature_csv_activation.py
|
This pull request has conflicts, please rebase. |
Munkybooty
force-pushed
the
backports-0.18-pr20
branch
from
cd176e8 to
b82a67b
Compare
Member
|
c++20 build fails + tests fail |
|
This pull request has conflicts, please rebase. |
faaac5c RPCTypeCheck bip32derivs arg in walletcreatefunded (Gregory Sanders) 1f0c428 QA: add basic walletcreatefunded optional arg test (Gregory Sanders) 1f18d7b walletcreatefundedpsbt: remove duplicate replaceable arg (Gregory Sanders) 2252ec5 Allow ConstructTransaction to not throw error with 0-input txn (Gregory Sanders) Pull request description: 1) Previously an empty input argument transaction that is marked for replaceability fails to pass the `SignalsOptInRBF` check right before funding it. Explicitly check for that condition before throwing an error. 2) The rpc call had two separate `replaceable` arguments, each of which being used in mutually exclusive places. I preserved the `options` version to retain compatability with `fundtransaction`. Tree-SHA512: 26eb0c9e2d38ea51d11f741d61100223253271a084adadeb7e78c6d4e9004636f089e4273c5bf64a41bd7e9ff795317acf30531cb36aeb0d8db9304b3c8270c3
Munkybooty
force-pushed
the
backports-0.18-pr20
branch
from
b82a67b to
8ddf9db
Compare
Author
|
c++20 build fixed by rebase, need to still investigate these test failures |
UdjinM6
requested changes
Mar 7, 2022
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
pls see below + https://github.com/UdjinM6/dash/commits/pr4675
There was a problem hiding this comment.
14738: it fixed an issue we never had (14732), pls drop it (and ea64227, 8ddf9db).
…mportprivkey a6b5ec1 rpc: creates possibility to preserve labels on importprivkey (marcoagner) Pull request description: Closes bitcoin#13087. As discussed in the issue, this is a feature request instead of a bug report since the behaviour was as intended (i.e. label with default: `''`). With this, the old behaviour is kept while the possibility to achieve the preservation of labels, as expected in the open issue, is added. Tree-SHA512: b33be50e1e7f62f7ddfae953177ba0926e2d848961f9fac7501c2b513322c0cb95787745d07d137488267bad1104ecfdbe800c6747f94162eb07c976835c1386
Munkybooty
force-pushed
the
backports-0.18-pr20
branch
from
0593e4e to
d6c31a4
Compare
UdjinM6
requested changes
Mar 8, 2022
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
LGTM, pls drop all commits related to 14738 (3b6848a, f82c23d, d4bafcb, cc86ef8, 71e16e2, 9d625c7) and it should be ready to merge imo
fa38d3d [rpc] Correct reconsiderblock help text, add test (MarcoFalke) Pull request description: Rework documentation and test to match the implementation Tree-SHA512: d0adef6b054a341bcc1cb87783a4e4cf9be124ba6812e1ac88246a5e01b2861a8071b12dba880b2b428c37da3fa860bfec3fe3e5fbb7c28696872113faa84a9f
fab17e8 test: Add basic test for BIP34 (MarcoFalke) Pull request description: BIP34 was disabled for testing, which explains why it had no test. Fix that by enabling it and adding a test. Tree-SHA512: 9cb5702d474117ce6420226eb93ee09d6fb5fc856fabc8b67abe56a088cd727674e0e5462000e1afa83b911374036f90abdbdde56a8c236a75572ed47e10a00f
2d5f1ea [tests] move wallet util functions to wallet_util.py (John Newbery) 6be64ef [tests] tidy up wallet_importmulti.py (John Newbery) Pull request description: Cherry picks un-merged commits from bitcoin#14952, which "fixes review comments from @ryanofsky here: bitcoin#14886 (review)" Tree-SHA512: 5f389196b0140d013a533d500f1812786a3a5cfb65980e13eaeacc459fddb55f43d05da3ab5e7cc8c997f26c0b667eed081ab6de2d125e631c70a7dd4c06e350
b745e14 [docs] Expand help text for importmulti changes (John Newbery) Pull request description: Expands the RPC help text for changes to the importmulti RPC method. Tree-SHA512: e90e5abf66bba3863e7519b5f79c26d18a4d624e6e7878293bdd4ebb57f1a01c67de52e4a5621901a8cb87fb3516264b3b1a826997c7c3c17b11216f1f1a3db0
fa5f890 rpc: Compile on GCC4.8 (MarcoFalke) Pull request description: GCC 4.8 is lacking some C++11 signatures (see "Adjust C++11 signatures to take a const_iterator." in GCC 4.9: gcc-mirror/gcc@3d2b2f4) Fix that by changing the code to use the pre-GCC 4.9 signature. Can be reverted after bitcoin#13356. Fixes bitcoin#15172 (reports on `Linux Mint 17.3 Rosa` and `CentOS Linux release 7.5.1804 (Core)`) Tree-SHA512: 0c0b18968270ad4fcd0c2000c57485be881a461135dac3ad0bdab22c1a2292cf6b28ebeb930ccaa0290ff20ce87547fd07ab8189c4c4fb54d652a3d0bc9615f8
Munkybooty
force-pushed
the
backports-0.18-pr20
branch
from
d6c31a4 to
ec162d7
Compare
PastaPastaPasta
approved these changes
Mar 11, 2022
Member
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK for merging via merge commit
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 16, 2025
, bitcoin#20640, bitcoin#24649, bitcoin#25118, bitcoin#25481, bitcoin#24699, bitcoin#25790, partial bitcoin#23201, bitcoin#11403 (wallet backports: part 7) 82413c8 merge bitcoin#25790: improve `{LoadActive,Deactivate}ScriptPubKeyMan` log (Kittywhiskers Van Gogh) 4f44f2b trivial: add recognition of OutputType::UNKNOWN for completeness (Kittywhiskers Van Gogh) bffeb91 trivial: realign AddAndGetDestinationForScript() signature with upstream (Kittywhiskers Van Gogh) 8473831 partial bitcoin#11403: SegWit wallet support (Kittywhiskers Van Gogh) bcb826e merge bitcoin#24699: Improve AvailableCoins performance by reducing duplicated operations (Kittywhiskers Van Gogh) a9214d4 merge bitcoin#25481: unify max signature logic (Kittywhiskers Van Gogh) 10ddbc3 merge bitcoin#25118: unify "allow/block other inputs" concept (Kittywhiskers Van Gogh) ab93b98 fix: don't call SelectionResult::AddInput() on every iteration (Kittywhiskers Van Gogh) 8b5132b merge bitcoin#24649: do not count wallet utxos as external (Kittywhiskers Van Gogh) 329b7b0 chore: adopt `RANDOM_CHANGE_POSITION` in Dash-specific code (Kittywhiskers Van Gogh) 580f4ca merge bitcoin#20640: return out-params of CreateTransaction() as optional struct (Kittywhiskers Van Gogh) 60d8d89 fix: remove unnecessary `ReserveDestination`s from wallet interface (Kittywhiskers Van Gogh) e0aa417 merge bitcoin#24859: Change wallet validation order (Kittywhiskers Van Gogh) 6758f1e partial bitcoin#23201: Allow users to specify input weights when funding a transaction (Kittywhiskers Van Gogh) 2e9e1ec merge bitcoin#23188: fund transaction external input cleanups (Kittywhiskers Van Gogh) c2766ec merge bitcoin#17211: Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs (Kittywhiskers Van Gogh) 4a5cf5b fix: add embedded addresses awareness and field to `getaddressinfo` (Kittywhiskers Van Gogh) 7af55a5 chore: extract `ScriptHash` specialization of `operator()` to function (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * The `embedded` field was introduced in [bitcoin#11403](bitcoin#11403) (3eaa003) for adding RPC awareness for P2SH (Legacy) SegWit addresses. While it is primarily used for SegWit, it can be used in non-SegWit contexts (e.g. atypical descriptor usage). As Dash Core does not support SegWit, this was not implemented and the leftover reference to `embedded` was removed from `getaddressinfo` in [dash#4675](#4675) (ec162d7). * But, [bitcoin#17211](bitcoin#17211) engages in this exact type of atypical descriptor usage in `rpc_psbt.py`, embedding a P2PKH address (which is valid in Dash Core) in a P2SH ([source](https://github.com/bitcoin/bitcoin/blob/928af61cdb2c4de1c3d10e6fda13bbba5ca0bba9/test/functional/rpc_psbt.py#L619-L620)). The exact scenario that was considered outside the scope of `getaddressinfo` but now causes a test failure (see below). <details> <summary>Test failure:</summary> ``` $ ./test/functional/rpc_psbt.py 2025-07-15T11:46:42.239000Z TestFramework (INFO): PRNG seed is: 8258839804852209703 2025-07-15T11:46:42.239000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_8e24i325 [...] 2025-07-15T11:47:02.519000Z TestFramework (INFO): PSBT with signed, but not finalized, inputs should have Finalizer as next 2025-07-15T11:47:03.553000Z TestFramework (ERROR): Key error Traceback (most recent call last): File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_framework.py", line 161, in main self.run_test() File "/home/eclair/Repositories/dashpay/dash/./test/functional/rpc_psbt.py", line 490, in run_test psbt = self.nodes[1].walletcreatefundedpsbt([ext_utxo], {self.nodes[0].getnewaddress(): 15}, 0, {'add_inputs': True, "solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}}) ~~~~~~~~~^^^^^^^^^^ KeyError: 'pubkey' 2025-07-15T11:47:03.605000Z TestFramework (INFO): Stopping nodes [...] ``` </details> This has since been remedied. * Since [bitcoin#16208](bitcoin#16208) (78fdc99), neither `CreateTransaction()` nor `CommitTransaction()` have consumed a `ReserveDestination` (then `CReserveKey`) but they have still persisted despite serving no purpose. `CreateTransaction()`, through `CreateTransactionInternal()` generates its own `ReserveDestination` ([source](https://github.com/dashpay/dash/blob/9310ebc43cdc4ccde7b3033f4d18139258d7c7ad/src/wallet/spend.cpp#L689)) and `CommitTransaction()` has no use for it. This has since been remedied. * When backporting [bitcoin#20640](bitcoin#20640), upstream took the decision to define `RANDOM_CHANGE_POSITION` at every location it was used, while unclear why this is the case, this is somewhat feasible for them. Dash Core, courtesy of CoinJoin, references the value significantly more often. So, we deviate from upstream and define `RANDOM_CHANGE_POSITION` in `wallet/spend.h` to consolidate definitions. * Dash-specific code has been updated to use `RANDOM_CHANGE_POSITION` when possible to specify _what_ the magic number (`-1`) is supposed to signify. * This deviation from upstream will resolve itself when backporting 758501b from [bitcoin#25273](bitcoin#25273), which does away with `RANDOM_CHANGE_POSITION` altogether. * When translating `!fRequireAllInputs` behavior (introduced in [dash#2581](#2581)) for [bitcoin#22019](bitcoin#22019) (c5038c1), it engaged in erroneous behavior of adding the whole set of `preset_inputs` to `result` _while_ iterating instead of _after_ ([source](https://github.com/dashpay/dash/blob/9310ebc43cdc4ccde7b3033f4d18139258d7c7ad/src/wallet/spend.cpp#L452-L457)). This results in unwanted accumulative behavior but did not surface as `SelectionResult` internally uses a `std::set` ([source](https://github.com/dashpay/dash/blob/9310ebc43cdc4ccde7b3033f4d18139258d7c7ad/src/wallet/coinselection.h#L278-L279)), which meant accumulative behavior was mitigated by deduplication. But this is an implementation detail and should not be relied for correctness of malformed code. To remedy this, the code has been reworked to call `SelectionResult::AddInput()` _after_ `nTargetValue` is met and a test, `minimum_inputs_test` has been introduced to `coinselector_tests` to validate that: * `!fRequireAllInputs` will result in _only_ consuming the minimum required set of coin to match `nTargetValue` **and** * The selected coins are accounted for correctly (i.e. `GetSelectedValue()` doesn't count the same coin multiple times) * To preserve the expected behavior of `!m_allow_other_inputs && !fRequireAllInputs`, despite the goal of [bitcoin#25118](bitcoin#25118) to reduce `vCoins` usage, we retain it _only_ for the Dash-specific condition, falling back to upstream behavior that condition isn't met or if `nTargetValue` is not satisfied. * While Dash only supports P2PKH as the _primary_ address type (a.k.a. `OutputType::LEGACY`), upstream supports multiple with the introduction of SegWit and introduced helper functions to track those multiple address types. They were introduced in [bitcoin#11403](bitcoin#11403) and as Dash doesn't implement SegWit, it wasn't implemented. Though, despite that, the `outputtype.{cpp,h}` source files were introduced when backporting [bitcoin#17261](bitcoin#17261) (ed88ba7) to fulfil that backport. As there are backports that may require those helper functions (e.g. [bitcoin#25790](bitcoin#25790)), they were implemented from [bitcoin#11403](bitcoin#11403) but directly applied to `outputtype.{cpp,h}`. * As `OutputType::UNKNOWN` exists as a valid entry in `develop` ([source](https://github.com/dashpay/dash/blob/9310ebc43cdc4ccde7b3033f4d18139258d7c7ad/src/outputtype.h#L14)), the helper functions were modified to account for this valid enum value courtesy of f5649db from [bitcoin#25734](bitcoin#25734). Liberties were taken to ensure the smallest set of changes needed were implemented. ## Breaking Changes None expected. ## How Has This Been Tested? A full CoinJoin session run on 7194128f0d  ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 6825df7c28355e7e2a9a0a4c497a83748a2481b164589a21d4db887ffad5bbe52c31bed36fd7b5ea5657b5e01555847ad15213b9dd47849a0269ee6dc69a40c7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I fixed as much as i could in terms of test failures, but not everything passes on the functional tests