wallet: fix crash on double block disconnection #31757

pull furszy wants to merge 2 commits into bitcoin:master from furszy:2025_wallet_fix_disconnectBlock_state changing 2 files +41 −2
  1. furszy commented at 6:46 pm on January 29, 2025: member

    Before detailing the issue: This crash should not occur in production, as block events are queued and processed in order by the validation signal workers. This PR is just fixing brittle code.

    Issue: The wallet crashes if it processes the same block disconnection event twice in a row due to an incompatible coinbase transaction state. This happens because disconnectBlock provides TxStateInactive without the “abandoned” flag for coinbase transactions to SyncTransaction, while AddToWallet() internally modifies it to retain the abandoned state.

    The crash flow is as follows:

    1. On the first disconnection, the transaction state transitions from “confirmed” to “inactive,” bypassing the state equality check since the provided state differs. Then, AddToWallet internally updates the state to “inactive + abandoned”

    2. On the second disconnection, as we provide only the “inactive” state to SyncTransaction(), the state equality assertion fails and crashes the wallet.

    Reviewers Note: The crash can easily be replicated by cherry-picking the test commit in master.

  2. wallet: fix crash on double block disconnection
    The wallet crashes if it processes the same block disconnection event twice in a row due
    to an incompatible coinbase transaction state.
    This happens because 'disconnectBlock' provides 'TxStateInactive' without the "abandoned"
    flag for coinbase transactions to 'SyncTransaction', while 'AddToWallet()' internally
    modifies it to retain the abandoned state.
    
    The flow is as follows:
    1) On the first disconnection, the transaction state transitions from "confirmed" to
    "inactive," bypassing the state equality check since the provided state differs. Then,
    'AddToWallet' internally updates the state to "inactive + abandoned"
    
    2) On the second disconnection, as we provide only the "inactive" state
    to 'SyncTransaction()', the state equality assertion fails and crashes the wallet.
    46375b4ad4
  3. DrahtBot commented at 6:46 pm on January 29, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31757.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #30221 (wallet: Ensure best block matches wallet scan state by achow101)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. DrahtBot added the label Wallet on Jan 29, 2025
  5. test: wallet, add coverage for crash on duplicated disconnectBlock call 4229839b45
  6. furszy force-pushed on Jan 29, 2025
  7. DrahtBot added the label CI failed on Jan 29, 2025
  8. DrahtBot commented at 7:01 pm on January 29, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/36373842434

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  9. DrahtBot removed the label CI failed on Jan 29, 2025


furszy DrahtBot

Labels
Wallet


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-02-07 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me