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

Improve re-insertion of TXs from disconnected blocks into mempool #319

Merged
merged 9 commits into from
Jan 31, 2020

Conversation

pinheadmz
Copy link
Member

Closes #306

Port of bcoin-org/bcoin#917, although it hasn't been reviewed or merged yet over there. I think this is a bigger concern for Handshake since this codebase will represent 100% of the network, and chain reorgs are more likely on hsd in the early days.

The contextual checks for TXs with covenants re-entering mempool after a block disconnect has already been written in _handleReorg(), so for that case I just added a test.

The other cases ported over from bcoin are premature coinbase spends, and premature BIP68 sequence lock spends.

With these contextual checks in place, the "don't remove block if mempool empty" check can be safely removed.

This method used to just toss out any TX that might be problematic
when the chain is rewound. Instead, we can actually check the context
of each TX and allow them to remain in the mempool if they are still
valid. Those checks include coinbase maturity and sequence locks.
@pinheadmz pinheadmz requested a review from chjj December 22, 2019 21:24
@codecov-io
Copy link

codecov-io commented Dec 22, 2019

Codecov Report

Merging #319 into master will increase coverage by 0.71%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #319      +/-   ##
==========================================
+ Coverage   58.68%   59.39%   +0.71%     
==========================================
  Files         129      129              
  Lines       35795    35793       -2     
  Branches     6027     6027              
==========================================
+ Hits        21007    21261     +254     
+ Misses      14788    14532     -256
Impacted Files Coverage Δ
lib/mempool/mempool.js 52.56% <100%> (+9.73%) ⬆️
lib/blockchain/chain.js 60.75% <0%> (+0.3%) ⬆️
lib/covenants/namestate.js 87.37% <0%> (+0.4%) ⬆️
lib/blockchain/chaindb.js 64.87% <0%> (+1.03%) ⬆️
lib/covenants/bitfield.js 89.53% <0%> (+1.16%) ⬆️
lib/covenants/namedelta.js 87.06% <0%> (+1.99%) ⬆️
lib/primitives/covenant.js 72.01% <0%> (+2.29%) ⬆️
lib/utils/binary.js 58.97% <0%> (+2.56%) ⬆️
lib/mempool/contractstate.js 80.09% <0%> (+4.97%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c554a6f...fb88597. Read the comment docs.

@pinheadmz
Copy link
Member Author

Also thinking we can deal with Claims and AirdropProofs here too:

hsd/lib/mempool/mempool.js

Lines 414 to 416 in c554a6f

this.dropClaims();
this.dropAirdrops();
this.contracts.clear();

They can be re-evaluated on reorgs. Claims for example commit to a block hash so that needs to be re-checked.

@pinheadmz
Copy link
Member Author

Ok this latest commit adds the Claims and AirDrops back into the mempool after a reorg. This will require diligent review if we want to keep it. From what I can tell, there are no restrictions on AirDrops if the chain is rewound (unlike premature coinbase spends). The only restriction on Claims would be the inception time (RRSIG contains a timestamp which is compared to the chain tip) so I added that exception.

@pinheadmz
Copy link
Member Author

Forgot to include checks for block commitment of Claims re-entering mempool in last commit, that has been updated. Everything should be covered now.

@pinheadmz
Copy link
Member Author

Bonus TODO: this is getting pretty advanced, but Bitcoin Core will even hold on to the evicted TXs until after a reorg is complete, and then see if any are still ok (for example, a coinbase spend might still be valid after a reorg, just not during):

https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/txmempool.h#L766-L776

@chjj
Copy link
Contributor

chjj commented Jan 31, 2020

After looking at this for a while, I can't find anything wrong with it. Merging for now.

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.

TXs do not re-enter mempool when a block is disconnected
4 participants