Skip to content

fix(electrum): Don't ignore multiple coinbase txs#1090

Merged
notmandatory merged 2 commits intobitcoindevkit:release/0.28from
danielabrozzoni:issue/1051
Aug 22, 2023
Merged

fix(electrum): Don't ignore multiple coinbase txs#1090
notmandatory merged 2 commits intobitcoindevkit:release/0.28from
danielabrozzoni:issue/1051

Conversation

@danielabrozzoni
Copy link
Copy Markdown
Member

We would previously insert just one coinbase transaction in the database if we caught multiple in the same sync.
When we sync with electrum, before committing to the database, we remove from the update conflicting transactions, using the make_txs_consistent function. This function considers two txs to be conflicting if they spend from the same outpoint - but every coinbase transaction spends from the same outpoint!
Here we make sure to avoid filtering out coinbase transactions, by adding a check on the txid just before we do the filtering.

Fixes #1051

Changelog notice

  • Fix a bug when syncing coinbase utxos on electrum

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

We would previously insert just one coinbase transaction in the database
if we caught multiple in the same sync.
When we sync with electrum, before committing to the database, we remove
from the update conflicting transactions, using the
`make_txs_consistent` function. This function considers two txs to be
conflicting if they spend from the same outpoint - but every coinbase
transaction spends from the same outpoint!
Here we make sure to avoid filtering out coinbase transactions, by
adding a check on the txid just before we do the filtering.

Fixes bitcoindevkit#1051
- reqwest 0.11.19 has MSRV 1.63.0+, pin to 0.11.18
- h2 0.3.21 has MSRV 1.63.0+, pin to 0.3.20
- rustls-webpki has MSRV 1.60.0+, pin to 0.100.1
Copy link
Copy Markdown
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

tACK 530ba36

I tested without the fix to confirm the 2nd coinbase isn't included and with the fix it is.

Thanks for the clear explanation and concise fix.

@notmandatory notmandatory added bug Something isn't working ci labels Aug 22, 2023
@notmandatory notmandatory merged commit e3ca356 into bitcoindevkit:release/0.28 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

invalid balance for wallet with multiple coinbase transactions

2 participants