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#17585, #17804, #17819, #18122, #18192, #18208, #18234, #18292, #18305, #18314, partial #18224 #5373

Merged
merged 11 commits into from
Jun 1, 2023
Merged

backport: bitcoin#17585, #17804, #17819, #18122, #18192, #18208, #18234, #18292, #18305, #18314, partial #18224 #5373

merged 11 commits into from
Jun 1, 2023

Conversation

knst
Copy link
Collaborator

@knst knst commented May 15, 2023 •
edited
Loading

Issue being fixed or feature implemented

Next batch of backports from v20 bitcoin.

What was done?

How Has This Been Tested?

Run functional/unit tests.

Breaking Changes

Please, notice that all addresses in RPC examples are unified and changed to invalid.

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 knst force-pushed the bc-bp-v20-missing-6 branch 2 times, most recently from 822b030 to 4c9b4e6 Compare May 16, 2023 12:24
@knst knst changed the title backport: bitcoin v20 batch 6 backport: bitcoin#17585, #17725, #17804, #17819, #18122, #18192, #18208, #18224, #18234, #18292, #18305, #18314 May 16, 2023
@knst knst added this to the 20 milestone May 16, 2023
@knst knst added the RPC Some notable changes to RPC params/behaviour/descriptions label May 16, 2023
@knst knst force-pushed the bc-bp-v20-missing-6 branch from 4c9b4e6 to 0a8528e Compare May 16, 2023 12:31
@knst
Copy link
Collaborator Author

knst commented May 16, 2023

somehow I am not sure how to create invalid psbt tx for functional test of finalizer (Merge bitcoin#18224).

My instruction and steps from #5170 doesn't work for this case.

I marked that backport as partial ATM.

@knst knst changed the title backport: bitcoin#17585, #17725, #17804, #17819, #18122, #18192, #18208, #18224, #18234, #18292, #18305, #18314 backport: bitcoin#17585, #17725, #17804, #17819, #18122, #18192, #18208, #18234, #18292, #18305, #18314, partial #18224 May 16, 2023
@knst knst force-pushed the bc-bp-v20-missing-6 branch 2 times, most recently from 5ed1ec7 to fcfc965 Compare May 21, 2023 10:17
@knst knst changed the title backport: bitcoin#17585, #17725, #17804, #17819, #18122, #18192, #18208, #18234, #18292, #18305, #18314, partial #18224 backport: bitcoin#17585, #17804, #17819, #18122, #18192, #18208, #18234, #18292, #18305, #18314, partial #18224 May 25, 2023
@knst knst marked this pull request as ready for review May 25, 2023 19:43
UdjinM6
UdjinM6 approved these changes May 27, 2023
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.

LGTM, utACK

@knst knst requested a review from PastaPastaPasta May 30, 2023 12:02
PastaPastaPasta
PastaPastaPasta approved these changes May 31, 2023
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 for merging via merge commit

MarcoFalke and others added 11 commits May 31, 2023 18:14
@PastaPastaPasta
Merge bitcoin#17819: doc: developer notes guideline on RPCExamples ad…
df51c19 
…dresses

42ec499 doc: developer notes guideline on RPCExamples addresses (Jon Atack)

Pull request description:

  to make explicit the use of invalid addresses for user safety and to encourage
  the use of bech32 addresses by default. See bitcoin#17578 (comment) and bitcoin#17578 (comment).

  Fix a typo to appease the linter.

ACKs for top commit:
  promag:
    ACK 42ec499, no strong opinion as whether this belongs to developer notes or not but why not.
  fjahr:
    ACK 42ec499
  michaelfolkson:
    ACK 42ec499

Tree-SHA512: 64f90e227d256aa194c4fd48435440bdc233a51213dd4a6ac5b05d04263f729c6b4bb5f3afd3b87719b20cb1b159d5a9673d58a11b72823a4a6a16e8a26ae10e
@theStack @PastaPastaPasta
Merge bitcoin#18122: rpc: update validateaddress RPCExamples to bech32
c06e887 
rpc: update validateaddress RPCExamples to bech32

also contains the following changes:
- rpc: factor out example bech32 address for RPCExamples
- doc: update developer notes wrt RPCExamples addresses
 (mention the EXAMPLE_ADDRESS constant as an example for an invalid bech32
  address suitable for RPCExamples help documentation)
@PastaPastaPasta
Merge bitcoin#18208: rpc: Change RPCExamples to bech32
52581d3 
3e32499 Change example addresses to bech32 (Yusuf Sahin HAMZA)

Pull request description:

  This is a follow-up PR to bitcoin#18197 that fixes RPCExamples.

  Fixes bitcoin#18185.

ACKs for top commit:
  MarcoFalke:
    ACK 3e32499
  jonatack:
    ACK 3e32499

Tree-SHA512: c7a6410ef8b6e169016c2c5eac3e6b9501caabd0e8a0871ec31e56bfc44589f056d3f5cb55b5a13bba36f6c15136c2352f883e30e4dcc0997ffd36b27f9173b9
@laanwj @PastaPastaPasta
Merge bitcoin#17804: doc: Misc RPC help fixes
4e62a8b 
fa5c662 doc: Use proper RPC help syntax in importmulti (MarcoFalke)
fab6311 doc: Remove duplicate "comment" from listsinceblock RPC help (MarcoFalke)
fa04cd6 doc: Properly document proxy_randomize_credentials as bool in getnetworkinfo (MarcoFalke)
fa9dec7 doc: Fix syntax error (trailing square bracket) in finalizepsbt (MarcoFalke)
faff5a6 doc: Fix syntax error (trailing square bracket) in walletprocesspsbt (MarcoFalke)
fa05459 doc: Add missing "optional" to "long" estimaterawfee RPC help (MarcoFalke)

Pull request description:

  This fixes documentation of the following RPCs:

  * estimaterawfee (hidden)
  * https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/walletprocesspsbt/
  * https://bitcoincore.org/en/doc/0.19.0/rpc/rawtransactions/finalizepsbt/
  * https://bitcoincore.org/en/doc/0.19.0/rpc/network/getnetworkinfo/
  * https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/listsinceblock/
  * https://bitcoincore.org/en/doc/0.19.0/rpc/wallet/importmulti/

  <!-- Also, it comes with a scripted diff to normalize whitespace and type names. (Previous attempts: bitcoin#14601 and bitcoin#14459)

ACKs for top commit:
  laanwj:
    ACK fa5c662

Tree-SHA512: 5a10956e12f8ce23e93a2ce8bafd6cae759d8a21658f79397e3bfce3e4aabd9658bdbd40acde49323dca958a9befee7166654994208c182dd60f483109621e17
@meshcollider @PastaPastaPasta
Merge bitcoin#17585: rpc: deprecate getaddressinfo label
984aae4 
d3bc184 doc: update release notes with getaddressinfo label deprecation (Jon Atack)
72af93f test: getaddressinfo label deprecation test (Jon Atack)
d48875f rpc: deprecate getaddressinfo label field (Jon Atack)
dc0cabe test: remove getaddressinfo label tests (Jon Atack)
c7654af doc: address pr17578 review feedback (Jon Atack)

Pull request description:

  This PR builds on bitcoin#17578 (now merged) and deprecates the rpc getaddressinfo `label` field. The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=label`.

  See http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622 and bitcoin#17283 (comment) for more context.

  Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=label` flag while verifying the rpc getaddressinfo output and help text.

  Next step: add support for multiple labels.

ACKs for top commit:
  jnewbery:
    ACK d3bc184
  laanwj:
    ACK d3bc184
  meshcollider:
    utACK d3bc184

Tree-SHA512: f954402884ec54977def332c8160fd892f289b0d2aee1e91fed9ac3220f7e5b1f7fc6421b84cc7a5c824a0582eca4e6fc194e4e33ddd378c733c8941ac45f56d
@PastaPastaPasta
Merge bitcoin#18192: Bugfix: Wallet: Safely deal with change in the a…
cedb78e 
…ddress book

b5795a7 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr)
6d2905f Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr)
c751d88 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr)
8e64b8c Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr)
65b6bdc Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr)
144b2f8 Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr)
b86cd15 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr)

Pull request description:

  In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.

  This no longer holds true after bitcoin#13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.

  Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).

  Fixing it is accomplished by:

  * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
  * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them.
  * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile).

  A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`.

ACKs for top commit:
  ryanofsky:
    Code review ACK b5795a7. Pretty clever and nicely implemented fix!
  jonatack:
    ACK b5795a7 nice improvements -- code review, built/ran tests rebased on current master ff53433 and tested manually with rpc/cli
  jnewbery:
    Good fix. utACK b5795a7.

Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0
@knst @PastaPastaPasta
Merge bitcoin#18234: refactor: Replace boost::mutex,condition_var,chr…
c0ba1e3 
…ono with std equivalents in scheduler
@PastaPastaPasta
Merge bitcoin#18292: fuzz: Add assert(script == decompressed_script)
c2b4dd6 
fab0e5b fuzz: Add assert(script == decompressed_script) (MarcoFalke)

Pull request description:

  Presumably an oversight in bitcoin#17926 (comment)

ACKs for top commit:
  practicalswift:
    Tested ACK fab0e5b

Tree-SHA512: 6dcec06169df497a540fd6ebbcd89f5db22257241b2bbe756de868742f9bc324b80d38dbababfa07e5f3a830aaae9fc6d168dcc2ca5d75da437bdf4dc4e0f370
@PastaPastaPasta
Merge bitcoin#18305: test: Explain why test logging should be used
87de1bc 
ffff9dc test: Explain why test logging should be used (MarcoFalke)

Pull request description:

  Background is that some tests don't have any `self.log` call at all. Thus there are no "anchor points" and those tests are hard to debug because the logs can't easily be parsed by a human.

ACKs for top commit:
  jonatack:
    ACK ffff9dc
  instagibbs:
    ACK bitcoin@ffff9dc
  fanquake:
    re-ACK ffff9dc

Tree-SHA512: 08d962e85c4892c2a0c58feb5dc697c680a9d68e41a79417da6fcd415e0c5c735c4533a985cf225bb89deb5ca717d9bedf990657958079185804caa512b10f5a
@knst @PastaPastaPasta
Merge bitcoin#18314: tests: Add deserialization fuzzing of SnapshotMe…
a5778e1 
…tadata (utxo_snapshot). Increase fuzzing coverage.
@meshcollider @PastaPastaPasta
partial Merge bitcoin#18224: Make AnalyzePSBT next role calculation s…
68dfc06 
…imple, correct

1ef28b4 Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)

Pull request description:

  Sniped test and alternative to bitcoin#18220

  Sjors documenting the issue:
  ```
  A PSBT signed by ColdCard was analyzed as follows (see bitcoin#17509 (comment))

  {
    "inputs": [
      {
        "has_utxo": true,
        "is_final": false,
        "next": "finalizer"
      }
    ],
    "estimated_vsize": 141,
    "estimated_feerate": 1e-05,
    "fee": 1.41e-06,
    "next": "signer"
  }
  I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
  ```

  It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.

  Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.

ACKs for top commit:
  Sjors:
    ACK 1ef28b4, much nicer. Don't forget to document the bug fix.
  achow101:
    ACK 1ef28b4
  Empact:
    ACK bitcoin@1ef28b4

Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
@UdjinM6 UdjinM6 merged commit 89fcecb into dashpay:develop Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Milestone
20
Development

Successfully merging this pull request may close these issues.
6 participants
@knst @UdjinM6 @PastaPastaPasta @theStack @laanwj @meshcollider