Skip to content

backport: Merge bitcoin#18554 #6715

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

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

vijaydasmp
Copy link

backport

…s chains

5f21321 tests: add tests for cross-chain wallet use prevention (Seibart Nedor)
9687659 wallet: ensure wallet files are not reused across chains (Seibart Nedor)

Pull request description:

  This implements a proposal in bitcoin#12805 and is a rebase of bitcoin#14533.

  This seems to be a working approach, but I'm not sure why the `p2p_segwit.py` functional test needed a change, so I'll look into it more.

ACKs for top commit:
  achow101:
    ACK 5f21321
  dongcarl:
    Code Review ACK 5f21321
  [deleted]:
    tACK bitcoin@5f21321

Tree-SHA512: 2c934300f113e772fc31c16ef5588526300bbc36e4dcef7d77bd0760c5c8f0ec77f766b1bed5503eb0157fa26dc900ed54d2ad1b41863c1f736ce5c1f3b67bec
@vijaydasmp vijaydasmp changed the title backport : Merge bitcoin#18554 backport: Merge bitcoin#18554 Jun 8, 2025
MarcoFalke and others added 3 commits June 8, 2025 20:55
fafe06c bench: Sort bench_bench_bitcoin_SOURCES (MarcoFalke)
fa31dc9 bench: Add logging benchmark (MarcoFalke)

Pull request description:

  Might make finding performance bottlenecks or regressions (bitcoin#17218) easier.

  For example, fuzzing relies on disabled logging to be as fast as possible.

ACKs for top commit:
  dergoegge:
    ACK fafe06c

Tree-SHA512: dd858f3234a4dfb00bd7dec4398eb076370a4b9746aa24eecee7da86f6882398a2d086e5ab0b7c9f7321abcb135e7ffc54cc78e60d18b90379c6dba6d613b3f7
c0a5fce test: Add test for erase orphan tx conflicted by block (Hennadii Stepanov)
fa45bb2 test: Add test for erase orphan tx included by block (Hennadii Stepanov)
5c04978 test: Add test for erase orphan tx from peer (Hennadii Stepanov)

Pull request description:

  This PR adds test coverage for the following cases:
  - erase orphan transactions when a peer is disconnected
  - erase an orphan transaction when it is included in a new tip block
  - erase an orphan transaction when it is conflicted with other transactions included in a new tip block

  Found useful while working on bitcoin#19374.

ACKs for top commit:
  aureleoules:
    tACK c0a5fce (`make check` and `test/functional/test_runner.py`).
  kouloumos:
    ACK c0a5fce with a nit per bitcoin#19393 (comment).
  pg156:
    Reviewed to bitcoin@c0a5fce. Concept ACK. Agree due to the lack of RPC calls to inspect orphan pool, using `assert_debug_log` to match strings in log is a reasonable way to test.

Tree-SHA512: 98f8deeee2d1c588c7e28a82e513d4a18655084198369db33fe2710458251eeaffed030626940072d7576f57fcbf7d856d761990129e2ca9e372d2ccbd86d07d
…ducing duplicated operations

bc886fc Change mapWallet to be a std::unordered_map (Andrew Chow)
2723560 Change getWalletTxs to return a set instead of a vector (Andrew Chow)
9753286 Change mapTxSpends to be a std::unordered_multimap (Andrew Chow)
1f798fe wallet: Cache SigningProviders (Andrew Chow)
8a105ec wallet: Use CalculateMaximumSignedInputSize to indicate solvability (Andrew Chow)

Pull request description:

  While running my coin selection simulations, I noticed that towards the end of the simulation, the wallet would become slow to make new transactions. The wallet generally performs much more slowly when there are a large number of transactions and/or a large number of keys. The improvements here are focused on wallets with a large number of transactions as that is what the simulations produce.

  Most of the slowdown I observed was due to `DescriptorScriptPubKeyMan::GetSigningProvider` re-deriving keys every time it is called. To avoid this, it will now cache the `SigningProvider` produced so that repeatedly fetching the `SigningProvider` for the same script will not result in the same key being derived over and over. This has a side effect of making the function non-const, which makes a lot of other functions non-const as well. This helps with wallets with lots of address reuse (as my coin selection simulations are), but not if addresses are not reused as keys will end up needing to be derived the first time `GetSigningProvider` is called for a script.

  The `GetSigningProvider` problem was also exacerbated by unnecessarily fetching a `SigningProvider` for the same script multiple times. A `SigningProvider` is retrieved to be used inside of `IsSolvable`. A few lines later, we use `GetTxSpendSize` which fetches a `SigningProvider` and then calls `CalculateMaximumSignedInputSize`. We can avoid a second call to `GetSigningProvider` by using `CalculateMaximumSignedInputSize` directly with the `SigningProvider` already retrieved for `IsSolvable`.

  There is an additional slowdown where `ProduceSignature` with a dummy signer is called twice for each output. The first time is `IsSolvable` checks that `ProduceSignature` succeeds, thereby informing whether we have solving data. The second is `CalculateMaximumSignedInputSize` which returns -1 if `ProduceSignature` fails, and returns the input size otherwise. We can reduce this to one call of `ProduceSignature` by using `CalculateMaximumSignedInputSize`'s result to set `solvable`.

  Lastly, a lot of time is spent looking in `mapWallet` and `mapTxSpends` to determine whether an output is already spent. The performance of these lookups is slightly improved by changing those maps to use `std::unordered_map` and `std::unordered_multimap` respectively.

ACKs for top commit:
  Xekyo:
    ACK bc886fc
  furszy:
    diff re-reACK bc886fc

Tree-SHA512: fd710fe1224ef67d2bb83d6ac9e7428d9f76a67f14085915f9d80e1a492d2c51cb912edfcaad1db11c2edf8d2d97eb7ddd95bfb364587fb1f143490fd72c9ec1
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.

2 participants