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

Fix: Output index should be u32 to prevent clobbering when over 65536 outputs. #75

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

junderw
Copy link
Member

@junderw junderw commented Feb 4, 2024

Warning: REQUIRES RE-INDEX!!!!

Problem: If tx has over 16 bits of outputs, the indexing will clobber index 0 with index 65536 and so on.

Solution: 4 billion outputs is impossible without a hard fork. Use u32.

@junderw junderw requested a review from mononaut February 4, 2024 12:53
@junderw junderw force-pushed the junderw/fix-vout-u32 branch from 602c653 to 5b2075b Compare February 4, 2024 13:28
Copy link
Contributor

@mononaut mononaut left a comment

Choose a reason for hiding this comment

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

Tested ACK @ 5b2075b

I manually deleted the db on testnet, successfully reindexed, and confirmed that it stays up after new blocks, address lookups etc. Also checked that all of the major API endpoints still work as expected.

The excessive output bug actually didn't reproduce on master in my local environment, so I wasn't able to confirm this fixes that.

I also have not tested Liquid assets, which this PR seems to touch.

shesek added a commit to shesek/electrs that referenced this pull request Feb 15, 2024
Bitcoin's consensus rules allows for output indexes larger than u16,
which would result in an overflow prior to this fix.

This recently happened with a transaction on testnet:
https://blockstream.info/testnet/tx/ca3b75556430e1adf9e9790bce9c73a3d9afdb42305588e64c65b258c06c05c9

Based on @junderw's mempool/electrs#75. Thanks!

This change requires a full database reindex.
shesek added a commit to shesek/electrs that referenced this pull request Feb 20, 2024
Bitcoin's consensus rules allows for output indexes larger than u16,
which would result in an overflow prior to this fix.

This recently happened with a transaction on testnet:
https://blockstream.info/testnet/tx/ca3b75556430e1adf9e9790bce9c73a3d9afdb42305588e64c65b258c06c05c9

Based on @junderw's mempool/electrs#75. Thanks!

This change requires a full database reindex.
@natsoni
Copy link
Contributor

natsoni commented Feb 21, 2024

Tested ACK @ 5b2075b

Successfully reindexed both Liquid and testnet db. The out of range prevout bug, which reproduced locally on current electrs version, did not reproduced after reindexing on 5b2075b.

On liquid, everything seems to work fine with the new index (peg ins and assets notably)

@wiz wiz merged commit 583d9e9 into mempool Sep 4, 2024
7 checks passed
@wiz wiz deleted the junderw/fix-vout-u32 branch September 4, 2024 11:47
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.

4 participants