Skip to content

Conversation

xdustinface
Copy link

@xdustinface xdustinface commented Aug 22, 2020

I decided to add this because i worked on a fix for CreateTransaction and while doing that i figured its very hard to say for sure by just manual test/reviewing if things are still good since this is quite complex method imo with a lot possible routes. I think this should catch all currently possible cases in there. If someone can think about any improvement or more test cases, let me know!

Requires #3666 and #3668 to pass all the tests.

@xdustinface xdustinface marked this pull request as draft August 22, 2020 17:37
@xdustinface xdustinface marked this pull request as ready for review August 24, 2020 08:49
@xdustinface
Copy link
Author

Ready for review!

@PastaPastaPasta
Copy link
Member

You should probably move #3668 into this PR, cause unit tests need to be passing for us to merge this

@xdustinface
Copy link
Author

Requires #3666 and #3668 to pass all the tests.

Hmm imo separating them makes sense (depending on what will happen during review process :D). Why not just merge #3666 and #3668 first and then rebase this one as the others still pass without this PR. You can still verify #3668 locally with this PR for now. Still prefer a merge? Let me know!

@UdjinM6
Copy link

UdjinM6 commented Aug 24, 2020

I'd prefer rebasing this on top of develop after 3668 is merged.

@UdjinM6 UdjinM6 added this to the 16 milestone Aug 26, 2020
return false;
}
// Verify the number of requested outputs does match the resulting outputs
CheckEqual(vecEntries.size(), wtx.tx->vout.size() - (nChangePos != -1 ? 1 : 0));
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use BOOST_CHECK_EQUAL here?

Copy link
Author

@xdustinface xdustinface Sep 3, 2020

Choose a reason for hiding this comment

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

What about 29b9e0f? Saves one line :D

Comment on lines 857 to 862
BOOST_CHECK(CreateTransaction({{10, false}}, false));
BOOST_CHECK(CreateTransaction({{10, true}}, false));
BOOST_CHECK(CreateTransaction({{100, false}}, false));
BOOST_CHECK(CreateTransaction({{100, true}}, false));
BOOST_CHECK(CreateTransaction({{10000, true}, {100, true}}, false));
BOOST_CHECK(CreateTransaction({{10000, false}, {100, false}}, false));
Copy link
Member

Choose a reason for hiding this comment

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

probably should just dust - 1, dust, and dust+1

Copy link
Author

Choose a reason for hiding this comment

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

Hm i just dropped those initial tests, c7e34f5. They are anyway covered in the rest of the test cases.

@xdustinface xdustinface force-pushed the pr-test-createtransaction branch from c7e34f5 to 2190f80 Compare September 4, 2020 12:02
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

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

@UdjinM6 UdjinM6 merged commit 4510378 into dashpay:develop Sep 4, 2020
xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction

* test: Reset minRelayTxFee in CreateTransactionTest

At this point its modified from an other test when running all unit 
tests which lets this test fail unexpectedly.

* test: Lock cs_main/cs_wallet when required in CreateTransactionTest

* test: Add new test in CreateTransactionTest

Test if the wallet creation fails properly with the correct error
messages.

* test: Fixed expected test results for some CreateTransactionTest cases

* test: Fail if CreateTransaction runs into "Exceed max tries" case

* test: Adjust last return in CreateTransaction

* test: Drop dust tests
xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction

* test: Reset minRelayTxFee in CreateTransactionTest

At this point its modified from an other test when running all unit 
tests which lets this test fail unexpectedly.

* test: Lock cs_main/cs_wallet when required in CreateTransactionTest

* test: Add new test in CreateTransactionTest

Test if the wallet creation fails properly with the correct error
messages.

* test: Fixed expected test results for some CreateTransactionTest cases

* test: Fail if CreateTransaction runs into "Exceed max tries" case

* test: Adjust last return in CreateTransaction

* test: Drop dust tests
xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction

* test: Reset minRelayTxFee in CreateTransactionTest

At this point its modified from an other test when running all unit 
tests which lets this test fail unexpectedly.

* test: Lock cs_main/cs_wallet when required in CreateTransactionTest

* test: Add new test in CreateTransactionTest

Test if the wallet creation fails properly with the correct error
messages.

* test: Fixed expected test results for some CreateTransactionTest cases

* test: Fail if CreateTransaction runs into "Exceed max tries" case

* test: Adjust last return in CreateTransaction

* test: Drop dust tests
xdustinface added a commit to xdustinface/dash that referenced this pull request Sep 12, 2020
* test: Implement unit tests for CWallet::CreateTransaction

* test: Reset minRelayTxFee in CreateTransactionTest

At this point its modified from an other test when running all unit 
tests which lets this test fail unexpectedly.

* test: Lock cs_main/cs_wallet when required in CreateTransactionTest

* test: Add new test in CreateTransactionTest

Test if the wallet creation fails properly with the correct error
messages.

* test: Fixed expected test results for some CreateTransactionTest cases

* test: Fail if CreateTransaction runs into "Exceed max tries" case

* test: Adjust last return in CreateTransaction

* test: Drop dust tests
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 11, 2022
* test: Implement unit tests for CWallet::CreateTransaction

* test: Reset minRelayTxFee in CreateTransactionTest

At this point its modified from an other test when running all unit 
tests which lets this test fail unexpectedly.

* test: Lock cs_main/cs_wallet when required in CreateTransactionTest

* test: Add new test in CreateTransactionTest

Test if the wallet creation fails properly with the correct error
messages.

* test: Fixed expected test results for some CreateTransactionTest cases

* test: Fail if CreateTransaction runs into "Exceed max tries" case

* test: Adjust last return in CreateTransaction

* test: Drop dust tests
PastaPastaPasta pushed a commit that referenced this pull request Feb 9, 2024
…hify help text, undashify code comments (#5852)

## Issue being fixed or feature implemented
This pull request is a follow-up to
[some](#5834 (comment))
[feedback](#5834 (comment))
received on [dash#5834](#5834) as
the patterns highlighted were present in different parts of the codebase
and hence not corrected within the PR itself but addressed separately.

This is that separate PR 🙂 (with some additional cleanup of my own)

## What was done?
* This pull request will remain a draft until
[dash#5834](#5834) as it will
introduce more changes that will need to be corrected in this PR.
* Code introduced that is unique to Dash Core (CoinJoin, InstantSend,
etc.) has been excluded from un-Dashification as the purpose of it is to
reduce backport conflicts, which don't apply in those cases.
* `CWallet::CreateTransaction` and the `CreateTransactionTest` fixture
have been excluded as the former originates from
[dash#3668](#3668) and the latter
from [dash#3667](#3667) and are
distinct enough to be unique to Dash Core.
* There are certain Dashifications and SegWit-removals that prove
frustrating as it would break compatibility with programs that rely on
the naming of certain keys
* `getrawmempool`, `getmempoolancestors`, `getmempooldescendants` and
`getmempoolentry` return `vsize` which is currently an alias of `size`.
I have been advised to retain `vsize` in lieu of potential future
developments. (this was originally remedied in
219a1d0 but has since been dropped)
* `getaddressmempool`, `getaddressutxos` and `getaddressdeltas` all
return a value with the key `satoshis`. This is frustrating to rename to
`duffs` for compatibility reasons.
* `decodepsbt` returns (if applicable) `non_witness_utxo` which is
frustrating to rename simply to `utxo` for the same reason.
* `analyzepsbt` returns (if applicable) `estimated_vsize` which
frustrating to rename to `estimated_size` for the same reason.

## How Has This Been Tested?

## Breaking Changes
None

## Checklist:
- [x] 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
PastaPastaPasta added a commit that referenced this pull request Feb 12, 2025
 (wallet backports: part 1)

153bdc2 merge bitcoin#22155: Add test for subtract fee from recipient behavior (Kittywhiskers Van Gogh)
0185847 fix: correct fee calculations in `CreateTransactionInternal` (Kittywhiskers Van Gogh)
445f489 merge bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh)
7e54bd9 wallet: copy and sort `vecSend` if BIP69 sorting enabled, enable sorting (Kittywhiskers Van Gogh)
9e9e66f partial bitcoin#17331: Use effective values throughout coin selection (Kittywhiskers Van Gogh)
66fe2d4 merge bitcoin#25497: more accurate target for large transactions (Kittywhiskers Van Gogh)
6e4d789 wallet: add back missing `CoinSelectionParams` assignments (Kittywhiskers Van Gogh)
bd35042 wallet: move `CoinSelectionParams` to positions expected upstream (Kittywhiskers Van Gogh)
0711e67 wallet: shuffle transaction inputs if we aren't using BIP69 (Kittywhiskers Van Gogh)
1cf1c58 refactor: move selected coin and txin sorting to the end of the scope (Kittywhiskers Van Gogh)
ab756ba wallet: Fail if maximum weight is too large (Kittywhiskers Van Gogh)
05c319e refactor: move oversized transaction check to tail end of scope (Kittywhiskers Van Gogh)
6ca51df wallet: Use existing feerate instead of getting a new one (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6543

  * Dependency for #6529

  * [bitcoin#17331](bitcoin#17331) logically partially reverts [dash#3368](#3668) as Dash Core implemented a calculate-before approach (compared to _then_ Bitcoin Core's calculate-and-adjust approach) and it is being replaced  with the current upstream calculate-after approach done in a single-pass instead of iteratively (like the former two).
    * As the changes are non-trivial, they have been split into a "partial" and a "merge" commit, the first half dedicated just to the logical partial revert and the latter half dedicated to using effective values in coin selection.
      * BIP69 sorting is disabled in the former half to allow the fix to be in a separate commit while allowing validation of the remaining set of changes. The fix re-enables BIP69 sorting.
    * Due to the changes introduced in [dash#3368](#3668), a lot of then-redundant code was removed and changes to it upstream were not mirrored in Dash Core. To allow [bitcoin#17331](bitcoin#17331) to work properly, a lot of that unmirrored code was reintroduced and existing code readjusted to match upstream.

  * `coin_selection_params.tx_noinputs_size` is said to have a size (sans output count) of `9` instead of `10` as we don't have a SegWit field (referred to as `1 witness overhead (dummy, flag, stack size)` in a code comment) on account of not having SegWit.

  * To allow for backporting [bitcoin#17331](bitcoin#17331), portions of [bitcoin#21083](bitcoin#21083) (1a6a0b0) and [bitcoin#20536](bitcoin@51e2cd3) (3e69939) were backported.

  * [bitcoin#17331](bitcoin#17331) seems to have silently broken `CreateTransactionInternal` as functional tests fail (see below) despite the backport not intending to change behavior. This was caught due to unit tests introduced in [dash#3667](#3667).

    The aberration seems be remedied by portions of [bitcoin#25647](bitcoin#25647) and [bitcoin#26643](bitcoin#26643) and they have been incorporated into this pull request in a separate commit.

    **Special thanks to UdjinM6 for figuring this out!** 🎉

    <details>

    <summary>Error log:</summary>

    ```
     dash@479e0aa4ebbf:/src/dash$ ./src/test/test_dash -t coinselector_tests,wallet_tests
     Running 21 test cases...
     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 2 - 5

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 4 - 4

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 4 - 5

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 0

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 2

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 4

     wallet/test/wallet_tests.cpp(749): error: in "wallet_tests/CreateTransactionTest": check expected == actual has failed [false != true]
     CreateTransactionTest failed at: 6 - 5

     *** 7 failures are detected in the test module "Dash Core Test Suite"
    ```

    </details>

  ## How Has This Been Tested?

  153bdc2 was tested on Debian 12 (`bookworm`) mixing ~2 tDASH on default settings.

  ![CoinJoin run](https://github.com/user-attachments/assets/da1f13e7-dd83-4211-8d42-0cd4c770bbf1)

  ## Breaking Changes

  * If a transaction isn't shuffled using BIP69 (i.e. if an explicit position for the change txout is specified), it will be randomly shuffled (mirroring upstream behavior, [source](https://github.com/bitcoin/bitcoin/blob/51a3ac242c92e69b59df26f8f9e287b31e5c3b0f/src/wallet/wallet.cpp#L3048)). This deviates from earlier behavior where no shuffling would be done at all if BIP69 isn't applied.

  ## 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)_

ACKs for top commit:
  UdjinM6:
    utACK 153bdc2
  PastaPastaPasta:
    utACK 153bdc2

Tree-SHA512: 709b77dce3cea2bbf09eab42c7e70171c3bc6527ff4c387a4db75994da5c0d59025b641d90f734b87a62cdfa8e422d09513697a6e875635de2765a1c9141233e
PastaPastaPasta added a commit that referenced this pull request Feb 25, 2025
, bitcoin#22742, bitcoin#19101, bitcoin#22183, bitcoin#22009, bitcoin#22938, bitcoin#23288, bitcoin#24592 (wallet backports: part 2)

e2eea71 merge bitcoin#24592: Delete old line of code that was commented out (Kittywhiskers Van Gogh)
cf8d5a3 merge bitcoin#23288: remove usage of LegacyScriptPubKeyMan from some wallet tests (Kittywhiskers Van Gogh)
e99d065 merge bitcoin#22938: Add remaining scenarios of 0 waste, in wallet waste_test (Kittywhiskers Van Gogh)
31f8f9e merge bitcoin#22009: Decide which coin selection solution to use based on waste metric (Kittywhiskers Van Gogh)
24695e1 merge bitcoin#22183: Remove `gArgs` from `wallet.h` and `wallet.cpp` (Kittywhiskers Van Gogh)
14aa05d merge bitcoin#19101: remove ::vpwallets and related global variables (Kittywhiskers Van Gogh)
0d64290 refactor: separate out Dash-specific RPCs that rely on wallet logic (Kittywhiskers Van Gogh)
1548058 partial bitcoin#19101: remove ::vpwallets and related global variables (Kittywhiskers Van Gogh)
1fcdc96 test: remove unneeded code from some `wallet_tests` (Kittywhiskers Van Gogh)
5dc5090 refactor: move tests to match upstream order, Dash tests at the tail (Kittywhiskers Van Gogh)
2c24c63 merge bitcoin#22742: Use proper target in do_fund_send (Kittywhiskers Van Gogh)
2f3ceba merge bitcoin#22686: Use GetSelectionAmount in ApproximateBestSubset (Kittywhiskers Van Gogh)
f94993c merge bitcoin#22008: Cleanup and refactor CreateTransactionInternal (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #6517

  * Dependent on #6594

  * [bitcoin#19101](bitcoin#19101) has been split into two due to it performing two refactors in one commit, the first half deals with utilizing interfaces and managers as defined in `WalletContext` (as opposed to either passing them in separately as arguments or accessing them through globals) and the second half deals with deglobalizing wallet variables like `::vpwallets` and `cs_wallets`.

    * `WalletContext` still needs to be initialized with a constructor as it stores a const-ref `std::unique_ptr` of `interfaces::CoinJoin::Loader`. This requirement hasn't been done away with. Tests that don't have access to `coinjoin_loader` will still need to pass `nullptr`.

    * Bitcoin registers wallet RPCs through `WalletClient::registerRpcs()` ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/interfaces.cpp#L512-L522)), which a part of the `ChainClient` interface ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/interfaces/chain.h#L292-L293)). They are enumerated ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/rpcwallet.cpp#L4702-L4776)) differently from non-wallet RPCs ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/rpc/blockchain.cpp#L2638-L2682)).

      Wallet RPCs aren't supposed to have knowledge of `NodeContext` and likewise non-wallet RPCs aren't supposed to have knowledge of `WalletContext`. So far, Bitcoin has reworked their RPCs to maintain this separation and upstream RPCs are a part of the `libbitcoin_wallet` library.

        This isn't the case for some Dash RPCs, many of which reside in `libbitcoin_server`, some of which fit into two categories.

      * Requires knowledge of both `NodeContext` and `WalletContext`
        * Knowledge of `WalletContext` wasn't mandatory before deglobalization as wallet information could be accessed through the global context courtesy of `::vpwallets` and friends but now that it is, many RPCs need to be able to access `WalletContext` when otherwise they wouldn't need to.
        * Due to the mutual exclusivity mentioned above, RPCs that access `WalletContext` _cannot_ access `NodeContext` as their RPC registration logic doesn't give them access to `NodeContext` ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/interfaces.cpp#L516-L517), `m_context` here is `WalletContext`) but in order to give those RPCs `WalletContext`, we need to register them as wallet RPCs, which prevent us from accessing `NodeContext`.
        * This has been **tentatively** worked around by including a pointer to `NodeContext` in `WalletContext` ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/wallet/context.h#L47-L52)) and modifying `EnsureAnyNodeContext()` to fetch from `WalletContext` if regular behavior doesn't yield a `NodeContext` ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/rpc/server_util.cpp#L28-L37)).

      * Are RPCs that possess both wallet and non-wallet functionality (i.e. have "reduced" capabilities if wallet support isn't compiled in as opposed to being absent altogether).
        * To enable wallet functionality, the RPCs need to be _registered_ as wallet RPCs. If wallet functionality is disabled, those RPCs are not registered. Simple enough, unless you have an RPC that can _partially_ work if wallet functionality as disabled, as the lack of registration would mean those RPCs are _entirely_ inaccessible.
        * This is also **tentatively** worked around by registering them as non-wallet RPCs in the cases where wallet RPCs aren't registered (i.e. if wallet support isn't compiled in **or** if wallet support is disabled at runtime).
          * Bitcoin doesn't use `#ifndef ENABLE_WALLET` for checking if support is absent, we're expected to use `!g_wallet_init_interface.HasWalletSupport()` (which will always evaluate `false` if support isn't compiled in, [source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/dummywallet.cpp#L19), as the stub wallet would be utilized to initialize the global in those cases, [source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/dummywallet.cpp#L57)). This has been done in the PR.
          * A notable change in this PR is that a lot of behavior that would be enabled if support was compiled in but disabled at runtime would now be disabled if support was disabled at runtime as `wallet_loader` won't be initialized if runtime disablement occurs ([source](https://github.com/bitcoin/bitcoin/blob/62a09a30772141ef4add2f10d29927211abf57eb/src/wallet/init.cpp#L128-L131)), which in turns means that anything that relies on `wallet_loader` wouldn't work either, such as `coinjoin_loader`.

             This means that we also need to register the RPC as a non-wallet RPC if support is compiled in but disabled at runtime ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/rpc/evo.cpp#L1841-L1843)).

    * To register RPCs as wallet RPCs, generally they're done through `WalletClient::registerRpcs()`, which iterates through `GetWalletRPCCommands()`. This is perfectly fine as both reside in `libbitcoin_wallet`. Unfortunately, our Dash RPCs reside in `libbitcoin_server` and `registerRpcs()` cannot be extended to enumerate through RPCs not included in its library.

      We cannot simply move the Dash RPCs either (at least in its current state) as they rely on non-wallet logic, so moving the source files for those RPCs into `libbitcoin_wallet` would pull more or less, all of `libbitcoin_server` along with it.
      * To **tentatively** get around this, a new method has been defined, `WalletLoader::registerOtherRpcs()`, which will accept any set of RPCs for registration ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/wallet/interfaces.cpp#L603-L606)) and we use it in RPC init logic ([source](https://github.com/dashpay/dash/blob/eea8e42e631575de2968b9c1205c453bad88b135/src/init.cpp#L1513-L1528)).

    * Some usage of `CoreContext` had to be removed ([source](b27d2dc#diff-157f588a09ff1da05b8c74acfcfb4da21917b6a97ed5d2e702228d621d23e66dL1024)) as without those removals, the test suite would crash.

      <details>

      <summary>Crash:</summary>

      ```
      dash@01fd4f6cfa52:/src/dash$ lldb ./src/test/test_dash
      (lldb) target create "./src/test/test_dash"
      Current executable set to '/src/dash/src/test/test_dash' (x86_64).
      (lldb) r -t wallet_tests
      Process 653232 launched: '/src/dash/src/test/test_dash' (x86_64)
      Running 17 test cases...
      unknown location(0): fatal error: in "wallet_tests/CreateTransactionTest": unknown type
      wallet/test/wallet_tests.cpp(1052): last checkpoint

      *** 1 failure is detected in the test module "Dash Core Test Suite"
      Process 653232 exited with status = 201 (0x000000c9)
      ```

      </details>

  * The assertion introduced in [bitcoin#22686](bitcoin#22686) ([source](https://github.com/achow101/bitcoin/blob/92885c4f69a5e6fc4989677d6e5be8a666fbff0d/src/wallet/spend.cpp#L781-L783)) has not been backported as this assertion is downgraded to an error in [bitcoin#26611](bitcoin#26611) and then further modified in [bitcoin#26643](bitcoin#26643) ([source](bitcoin@c1a84f1#diff-6e06b309cd494ef5da4e78aa0929a980767edd12342137f268b9219167064d13R1016-R1020)), portions of which are already included in [dash#6517](#6517) ([source](https://github.com/dashpay/dash/blob/012298acb071a45fa943653197ccc4f2c48a75a4/src/wallet/wallet.cpp#L3832-L3835)).

  * [bitcoin#23288](bitcoin#23288) finished what e3687f7 ([dash#6078](#6078)) started and `coinselection_tests` has now been realigned with upstream.
    * Dash-specific tests in `wallet_tests` (as introduced in [dash#3367](#3667)) have not been moved over to descriptor wallets and are still based on legacy wallets (to this effect, the old `AddKey` logic has been retained as `AddLegacyKey`, [source](https://github.com/dashpay/dash/blob/d4e8d1f1628933d9d9159fc261b22fc4427a1c7a/src/wallet/test/wallet_tests.cpp#L873-L879)).

      * To prevent merge conflicts, Dash-specific tests have been moved to the end of the source file and demarcated by a comment ([source](https://github.com/dashpay/dash/blob/d4e8d1f1628933d9d9159fc261b22fc4427a1c7a/src/wallet/test/wallet_tests.cpp#L868)).

  ## How Has This Been Tested?

  6813863 was tested on Debian 12 (`bookworm`) mixing ~1 tDASH on default settings.

  ![CoinJoin run](https://github.com/user-attachments/assets/bdc6a20b-a2f5-4826-96b2-a1954e579e00)

  ## Breaking Changes

  * The RPCs `coinjoin`, `coinjoinsalt`, `getpoolinfo`, `gobject list-prepared`, `gobject prepare`, `gobject vote-alias`, `gobject vote-many`, `masternode outputs`, `protx update*`, `protx register*` and `protx revoke` will no longer be available if wallet support is disabled at runtime. This is not a change in **functionality** as they were already inoperative with disabled wallets but is a change in **reporting** as they would not be available at all.

  ## 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)_

ACKs for top commit:
  UdjinM6:
    LGTM, utACK e2eea71
  PastaPastaPasta:
    utACK e2eea71

Tree-SHA512: b3237670c2c861a353ce2486a1f0932b0bfcc6df488592cff28b85a49ed96c4612fc88866d0808e6d2cf0d409e4b8e8120e605acd7367b4bf554009672d3e9ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants