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.

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

backport: merge bitcoin#19289, #19998, #17775, #19401, #20167, #20187, #19961, #21114, #21170 (auxiliary backports: part 12) #5842

Merged
merged 10 commits into from
Feb 6, 2024

Conversation

kwvg
Copy link
Collaborator

@kwvg kwvg commented Jan 24, 2024

@kwvg kwvg force-pushed the bps_jan_03 branch 2 times, most recently from 28acf75 to 2b04c1e Compare January 25, 2024 06:46
@kwvg kwvg marked this pull request as ready for review January 25, 2024 09:38
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Jan 25, 2024
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

Overall seems fine, but check my suggestion please

}
bool isSpendable(const CTxDestination& dest) override
{
LOCK(m_wallet->cs_wallet);
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe refactor it to avoid code duplication?

bool isSpendable(const CTxDestination& dest) override
{
    return isSpendable(GetScriptForDestination(dest));
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Out of scope of the backport, can be done separately.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
test/functional/wallet_send.py Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@@ -45,7 +46,7 @@ def set_test_params(self):

def mine_chain(self):
self.log.info('Create some old blocks')
for _ in range(200):
for t in range(TIME_GENESIS_BLOCK, TIME_GENESIS_BLOCK + 200 * 156, 156):
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you mention that it's missing changes from bitcoin#15383 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are not missing changes, these were intentional logic changes done when bitcoin#15383 was backported in dash#4638.

@UdjinM6 on Mar 6, 2022:
Have to bump mocktime on all nodes because otherwise other nodes will ban us for relaying blocks from the future.

test/functional/mining_basic.py Show resolved Hide resolved
src/rpc/blockchain.cpp Show resolved Hide resolved
src/bench/rpc_blockchain.cpp Show resolved Hide resolved
@knst knst added this to the 20.1 milestone Jan 25, 2024
doc/release-notes-5842.md Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/wallet_send.py Outdated Show resolved Hide resolved
test/functional/mining_basic.py Show resolved Hide resolved
doc/tor.md Show resolved Hide resolved
doc/tor.md Show resolved Hide resolved
src/rpc/blockchain.cpp Show resolved Hide resolved
@kwvg kwvg force-pushed the bps_jan_03 branch 2 times, most recently from ce9ff9b to 105519a Compare January 25, 2024 13:38
@kwvg kwvg changed the title backport: merge bitcoin#19289, #16378, #19998, #17775, #20179, #20167, #20187, #19961, #21114, #20998, #21170 (auxiliary backports: part 12) backport: merge bitcoin#19289, #16378, #19998, #17775, #20179, #19401, #20167, #20187, #19961, #21114, #20998, #21170 (auxiliary backports: part 12) Jan 25, 2024
@kwvg kwvg requested review from knst and UdjinM6 January 25, 2024 14:29
@kwvg kwvg changed the title backport: merge bitcoin#19289, #16378, #19998, #17775, #20179, #19401, #20167, #20187, #19961, #21114, #20998, #21170 (auxiliary backports: part 12) backport: merge bitcoin#19289, #19998, #17775, #19401, #20167, #20187, #19961, #21114, #20998, #21170 (auxiliary backports: part 12) Jan 29, 2024
UdjinM6
UdjinM6 previously approved these changes Feb 1, 2024
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

src/rpc/blockchain.cpp Show resolved Hide resolved
Copy link

github-actions bot commented Feb 1, 2024

This pull request has conflicts, please rebase.

@kwvg kwvg changed the title backport: merge bitcoin#19289, #19998, #17775, #19401, #20167, #20187, #19961, #21114, #20998, #21170 (auxiliary backports: part 12) backport: merge bitcoin#19289, #19998, #17775, #19401, #20167, #20187, #19961, #21114, #21170 (auxiliary backports: part 12) Feb 1, 2024
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.

rebase looks clean, re-utACK

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

Let's have a draft PR with removed backports due to breaking changes?

[rpc] don't automatically append inputs in walletcreatefundedpsbt bitcoin/bitcoin#16377
The ultimate send RPC bitcoin/bitcoin#16378
test: Fix intermittent issue in wallet_import_rescan bitcoin/bitcoin#20179

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 for merging via merge commit

@PastaPastaPasta PastaPastaPasta merged commit 9c38b35 into dashpay:develop Feb 6, 2024
8 of 11 checks passed
PastaPastaPasta added a commit that referenced this pull request Mar 8, 2024
, bitcoin#19969 (send rpc)

76a49ad merge bitcoin#19969: Send RPC bug fix and touch-ups (Kittywhiskers Van Gogh)
17e2168 merge bitcoin#20179: Fix intermittent issue in wallet_import_rescan (Kittywhiskers Van Gogh)
c0f6b55 merge bitcoin#16378: The ultimate send RPC (Kittywhiskers Van Gogh)
a5da10e merge bitcoin#16377: don't automatically append inputs in walletcreatefundedpsbt (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  Extracted from [dash#5842](#5842) due to breaking changes and placed into its own PR to be merged during new major version development cycle.

  ## Breaking Changes

  _(Taken from `release-notes-5861.md`)_

  - The `walletcreatefundedpsbt` RPC call will now fail with `Insufficient funds` when inputs are manually selected but are not enough to cover the outputs and fee. Additional inputs can automatically be added through the new `add_inputs` option.

  - The `fundrawtransaction` RPC now supports `add_inputs` option that when `false` prevents adding more inputs if necessary and consequently the RPC fails.

  - A new `send` RPC with similar syntax to `walletcreatefundedpsbt`, including support for coin selection and a custom fee rate. The `send` RPC is experimental and may change in subsequent releases. Using it is encouraged once it's no longer experimental: `sendmany` and `sendtoaddress` may be deprecated in a future release.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [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 76a49ad
  knst:
    utACK 76a49ad
  PastaPastaPasta:
    utACK 76a49ad

Tree-SHA512: 05c5fc8c67b5ac9a97d28f8585f457904f71aed4702a0ffb8ec32dfd8e7f54f5bfd4981d53329e518cc0d29b9c4e830221b8e1f0bc4099f957778be420b6fb1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants