Skip to content

Add an option to use specific address as a source of funds in protx rpc commands (otherwise use payoutAddress/operatorPayoutAddress) #2581

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
Merged
merged 11 commits into from
Dec 28, 2018
Merged

Add an option to use specific address as a source of funds in protx rpc commands (otherwise use payoutAddress/operatorPayoutAddress) #2581

merged 11 commits into from
Dec 28, 2018

Conversation

@UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Dec 25, 2018 •
edited
Loading

Alternative to #2579

Pros:

  • no need to mix (fundAdddress can be pre-funded via PS if needed though);
  • PS is a really bad option for register_fund;
  • easy to manage fundAdddress per masternode (monitor its usage etc.).

Cons:

  • will merge all inputs belonging to fundAdddress each time (but this is probably ok and shouldn't cause any issues in most cases);
  • need to keep enough coins in fundAdddress (but amounts are tiny).

@UdjinM6 UdjinM6 added this to the 13.0 milestone Dec 25, 2018
codablock
codablock reviewed Dec 27, 2018
View reviewed changes
codablock
codablock reviewed Dec 27, 2018
View reviewed changes
@UdjinM6 UdjinM6 changed the title Add an option to use specific address as a source of funds in protx rpc commands Add an option to use specific address as a source of funds in protx rpc commands (otherwise use payoutAddress/operatorPayoutAddress) Dec 27, 2018
@codablock
Copy link

Testing this locally and noticed that ProTxs fail when too many inputs are present for the given address. TX simply gets too large. Something like codablock@ef16993 helps. This might also be useful in general as we shouldn't merge inputs when not explicitly requested by a user.

@codablock @UdjinM6
Add fRequireAllInputs to CCoinControl and use it in FundSpecialTx
6aa13fc 
This avoids adding all inputs even though they are not needed.
@codablock
Copy link

codablock commented Dec 27, 2018 •
edited
Loading

Tests are failing. codablock@7239a3a and codablock@74a56ea should fix them.

@codablock @UdjinM6
Allow to specify empty operator payout address and simplify fee code …
6f88fc5 
…in update_service

DIP3 tests were failing to call "protx update_service" as it did not accept
an empty "operatorPayoutAddress".

This also simplifies handling of the feeSourceAddr.
@codablock @UdjinM6
Specify source for funds and fees in DIP3 tests
239e7f3
codablock
codablock approved these changes Dec 28, 2018
View reviewed changes
Copy link

@codablock codablock left a comment

Choose a reason for hiding this comment

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

slightly tested ACK. Tests are green now

@UdjinM6 UdjinM6 merged commit 0dce846 into dashpay:develop Dec 28, 2018
@thephez thephez mentioned this pull request Dec 28, 2018
Merged
@UdjinM6 UdjinM6 deleted the addrtofundspectx branch November 26, 2020 13:25
@kwvg kwvg mentioned this pull request Jul 15, 2025
Merged
5 tasks
PastaPastaPasta added a commit that referenced this pull request Jul 16, 2025
@PastaPastaPasta
Merge #6755: backport: merge bitcoin#17211, bitcoin#23188, bitcoin#24859
1b2cc07 
, 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 7194128

  ![CoinJoin session run on build 7194128](https://github.com/user-attachments/assets/faab0b40-63ec-4178-8623-b2e6d51f55cb)

  ## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@codablock codablock codablock approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

13.0

Development

Successfully merging this pull request may close these issues.

2 participants

@UdjinM6 @codablock