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: bitcoin#20773, #21169, #21798, #21810, #21892, #21922, #21931, #22004 #6159

Merged
merged 10 commits into from
Aug 13, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Jul 26, 2024

Issue being fixed or feature implemented

Regular backports from bitcoin v22

What was done?

see commits

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A

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 added this to the 21.1 milestone Jul 26, 2024
@knst knst force-pushed the bp-v22-p15 branch 2 times, most recently from abbc48d to dfa854c Compare July 29, 2024 06:34
@knst knst marked this pull request as ready for review July 29, 2024 13:48
UdjinM6
UdjinM6 previously approved these changes Jul 30, 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.

light ACK dfa854c

(wallets seems to be loading correctly)

WITH_LOCK(::cs_main, tx_pool.check(chainstate));
{
BlockAssembler::Options options;
options.nBlockMaxSize= fuzzed_data_provider.ConsumeIntegralInRange(0U, MaxBlockSize(true));
Copy link

Choose a reason for hiding this comment

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

21798: nit:

Suggested change
options.nBlockMaxSize= fuzzed_data_provider.ConsumeIntegralInRange(0U, MaxBlockSize(true));
options.nBlockMaxSize = fuzzed_data_provider.ConsumeIntegralInRange(0U, MaxBlockSize(true));

@@ -16,6 +16,7 @@
<ip>:<port>
[<ipv6>]:<port>
<onion>.onion:<port>
<i2p>.b32.i2p:<port>
Copy link
Member

@PastaPastaPasta PastaPastaPasta Aug 1, 2024

Choose a reason for hiding this comment

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

I'd prefer to have 21825 in it's own PR and we actually test it; basically not clear how this would affect release process, and we should spin up at least 1 i2p node that we run :)

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.

lgtm dfa854c except the i2p I'd like to be in another PR and tested :)

@knst knst changed the title backport: bitcoin#20773, #21169, #21798, #21810, #21825, #21892, #21922, #21931, #22004 backport: bitcoin#20773, #21169, #21798, #21810, #21892, #21922, #21931, #22004 Aug 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 034dbc9

@UdjinM6
Copy link

UdjinM6 commented Aug 2, 2024

rebased via GH GUI to fix "Check Merge Fast-Forward Only"

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 bd5bd72

MarcoFalke and others added 7 commits August 12, 2024 20:38
…g coverage from 65% to 70%.

545404e fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. (practicalswift)

Pull request description:

  Add RPC interface fuzzing.

  This PR increases overall fuzzing line coverage from [~65%](https://marcofalke.github.io/btc_cov/fuzz.coverage/) to ~70% 🎉

  To test this PR:

  ```
  $ make distclean
  $ ./autogen.sh
  $ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
  $ make -C src/ test/fuzz/fuzz
  $ FUZZ=rpc src/test/fuzz/fuzz
  ```

  See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for more information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).

  Happy fuzzing :)

ACKs for top commit:
  MarcoFalke:
    Concept ACK 545404e

Tree-SHA512: 35fc1b508af42bf480ee3762326b09ff2eecdb7960a1917ad16345fadd5c0c21d666dafe736176e5a848ff6492483c782e4ea914cd9000faf50190df051950fd
fa03d0a fuzz: Create a block template in tx_pool targets (MarcoFalke)
fa61ce5 fuzz: Limit mocktime to MTP in tx_pool targets (MarcoFalke)
fab646b fuzz: Use correct variant of ConsumeRandomLengthString instead of hardcoding a maximum size (MarcoFalke)
fae2c8b fuzz: Allow to pass min/max to ConsumeTime (MarcoFalke)

Pull request description:

  Relatively simple check to ensure a block can always be created from the mempool

ACKs for top commit:
  practicalswift:
    Tested ACK fa03d0a

Tree-SHA512: e613376ccc88591cbe594db14ea21ebc9b2b191f6325b3aa4ee0cd379695352ad3b480e286134ef6ee30f043d486cf9792a1bc7e44445c41045ac8c3b931c7ff
…_pool

99993f0 fuzz: Avoid excessively large min fee rate in tx_pool (MarcoFalke)

Pull request description:

  Any fee rate above 1 BTC / kvB is clearly nonsense, so no need to fuzz this.

  Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34078

ACKs for top commit:
  practicalswift:
    cr ACK 99993f0: patch looks correct despite no `fa` prefix in commit hash

Tree-SHA512: bd3651d354b13d889ad1708d2b385ad0479de036de74a237346eefad5dbfb1df76ec02b55ec00487ec598657ef6102f992302b14c4e47f913a9962f81f4157e6
faa0d94 fuzz: Avoid timeout in EncodeBase58 (MarcoFalke)

Pull request description:

  The complexity is O(N^2), so limit the size.

  Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34126

  Oss-Fuzz testcase for `rpc` fuzzer: https://github.com/bitcoin/bitcoin/files/6461382/clusterfuzz-testcase-minimized-rpc-4831734974775296.log

ACKs for top commit:
  practicalswift:
    cr ACK faa0d94: patch looks correct
  sipa:
    utACK faa0d94

Tree-SHA512: 57ad9de8d811b828982d09a586782fc8a62fa3685590301d58120e2249caa30a9dccd3abe0b47e00ea8482de705fe0edbed298ab8761ea0d29496b50ed2db5d7
fa397a6 ci: Bump cirrus fuzz CPUs to avoid timeout (MarcoFalke)

Pull request description:

ACKs for top commit:
  hebasto:
    ACK fa397a6, let's try it.

Tree-SHA512: 7e06dda66c71d76e5fd144f6b5bb10f0bcac72feb15bd0f400ef08ba4dcb92558319401ef5f9d3822376affceb2192df1903b3a79c0ab2d7283ca21454054dea
5252f86 fuzz: Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of #ifdef forests (practicalswift)
54549dd fuzz: RPC fuzzer post-merge follow-ups. Remove unused includes. Update list of fuzzed RPC commands. (practicalswift)

Pull request description:

  Various RPC fuzzer follow-ups:
  * Remove unused includes.
  * Update list of fuzzed RPC commands.
  * Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of `#ifdef` forests.

  Context: bitcoin#21169 (review)

ACKs for top commit:
  MarcoFalke:
    Concept ACK 5252f86

Tree-SHA512: 286d70798131706ffb157758e1c73f7f00ed96ce120c7d9dc849e672b283f1362df47b206cfec9da44d5debb5869225e721761dcd5c38a7d5d1019dc6c912ab2
bbbb518 fuzz: Speed up transaction fuzz target (MarcoFalke)

Pull request description:

  `hashBlock` and `include_addresses` are orthogonal, so no need to do an exhaustive "search".

  Might fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34491

ACKs for top commit:
  practicalswift:
    cr ACK bbbb518: patch looks correct, and `TxToUniv` surprisingly wide in the `transaction_fuzz_target` flame graph! Putting it on a diet makes sense.

Tree-SHA512: 1e7c30c7fecf96364a9a1597c0a22139389fdeb67db59f3c2c6fc088196e3332877b2865991a957980d542f99a2f48cc066dd7cc16c695a5113190fe06205089
laanwj and others added 3 commits August 12, 2024 20:38
489ebb7 wallet: make chain optional for CWallet::Create (Ivan Metlushko)
d73ae93 CWallet::Create move chain init message up into calling code (Ivan Metlushko)
44c430f refactor: Add CWallet:::AttachChain method (Russell Yanofsky)
e2a47ce refactor: move first run detection to client code (Ivan Metlushko)

Pull request description:

  This is a followup for bitcoin#20365 (comment)
  First part of a refactoring with overall goal to simplify `CWallet` and de-duplicate code with `wallettool`

  **Rationale**: split `CWallet::Create` and create `CWallet::AttachChain`.

  `CWallet::AttachChain` takes chain as first parameter on purpose. In future I suggest we can remove `chain` from `CWallet` constructor.

  The second commit is based on be164f9 from bitcoin#15719 (thanks ryanofsky)

  cc ryanofsky achow101

ACKs for top commit:
  ryanofsky:
    Code review ACK 489ebb7. Only changes since last review were adding a const variable declaration, and implementing suggestion not to move feerate option checks to AttachChain. Thanks for updates and fast responses!

Tree-SHA512: 00235abfe1b00874c56c449adcab8a36582424abb9ba27440bf750af8f3f217b68c11ca74eb30f78a2109ad1d9009315480effc78345e16a3074a1b5d8128721
@PastaPastaPasta PastaPastaPasta merged commit 60403ef into dashpay:develop Aug 13, 2024
7 of 15 checks passed
@knst knst deleted the bp-v22-p15 branch August 13, 2024 06:21
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.

5 participants