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

Fallback to building empty block to mine (if txpool is somehow in invalid state) #2451

Merged
merged 4 commits into from
Jan 23, 2019

Conversation

antiochp
Copy link
Member

Beam issue got me paranoid about our txpool...

Adding some error handling and fallback behavior to the build_block fn in mine_block.
We call prepare_mineable_transactions() on the txpool and there exists the possibility of this raising an error and failing to prepare some valid transactions.

This PR handles an error here by falling back to an empty vec of txs which will result in the building of an empty block.

This would be far preferable to jamming a mining node up with an invalid txpool and no way to recover.

Note: The txpool is "reconciled" against chain state every time a new block is accepted, so even if we do end up with an invalid txpool, it should resolve itself against current chain state as soon as the next block is found.

This PR will not change this existing behavior, but should give the mining node a chance to mine an empty block.

@antiochp antiochp added this to the 1.0.1 milestone Jan 22, 2019
let txs = tx_pool
.read()
.prepare_mineable_transactions()
.unwrap_or(vec![]);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding a big and loud error message to the logs here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and that will suppress the error itself, right?

Copy link
Contributor

@ignopeverell ignopeverell left a comment

Choose a reason for hiding this comment

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

Sounds like a good "belt and suspender" PR. Thoughts about clearing the pool to be able to accept transactions again? Downside is it'll make debugging harder after the fact.

@antiochp
Copy link
Member Author

I thought about clearing/resetting the pool but it feels like this would be potentially worse than leaving it alone.
We should resolve any problems when we next reconcile the pool (on accepting the next block).

And if we have a situation where someone was able to construct a tx that triggered this then they have the ability to drop existing txs on the floor with this.

Agree on the loud logging - let me update the error handling to do this.

@antiochp antiochp force-pushed the fallback_to_empty_block branch from e0680b2 to 61fe141 Compare January 23, 2019 10:50
@antiochp
Copy link
Member Author

Now logging loudly if we fail to prepare txs for mining (and we fallback to mining empty block).

@ignopeverell ignopeverell merged commit 2299a03 into mimblewimble:master Jan 23, 2019
@antiochp antiochp deleted the fallback_to_empty_block branch January 23, 2019 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants