As per title.
Depends on D2525.
Details
- Reviewers
deadalnix Fabien florian jasonbcox - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Commits
- rSTAGING8ff468e92bf4: Improved mempool handling when changing validation rulesets
rABC8ff468e92bf4: Improved mempool handling when changing validation rulesets
run abc-replay-protection.py and abc-mempool-coherence-on-activations.py
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
src/validation.cpp | ||
---|---|---|
2759 ↗ | (On Diff #7258) | updateMempoolForReorg can be necessary as it calls removeForReorg(), which removes immature transactions (too-early spends of coinbases, and nLockTime in future). In principle a reorg can take you to a lower height chain or an earlier MTP (if said chain has more chainwork) so you could end up with these in the mempool. |
test/functional/abc-replay-protection.py | ||
---|---|---|
9 ↗ | (On Diff #7267) | Don't have one thing do two things. |
test/functional/abc-replay-protection.py | ||
---|---|---|
9 ↗ | (On Diff #7267) | Hmm fair enough, maybe better phrasing "This can be used as a template for other upgrade/fork events that rely on mempool reprocessing when changing transaction validation rules." |
src/validation.cpp | ||
---|---|---|
2203 ↗ | (On Diff #7277) | Indeed! Fixed. |
Looks good on the functional test, you even did "Send post-fork-only txn into the mempool again" 👍
Honestly the test is completely unreadable. There is no way I can ensure this is correct.
test/functional/abc-mempool-coherence-on-activations.py | ||
---|---|---|
185 ↗ | (On Diff #7291) | this seems to overlap with create_fund_and_spend_tx . This test is very hard to follow because it's clearly the other test with some pieces retrofited. This also fails at the Liskov substitution principle. http://craftedsw.blogspot.com/2010/06/liskov-substitution-principle-lsp.html |
Improved readability of the functional test.
Moved activation specific code away from the main test function.
Renamed functions to better describe what they are doing.
Replaced all references of the specific activation to generic ones.
test/functional/abc-mempool-coherence-on-activations.py | ||
---|---|---|
31 ↗ | (On Diff #7308) | Nice way to separate the concerns. :-) |
test/functional/abc-mempool-coherence-on-activations.py | ||
---|---|---|
36 ↗ | (On Diff #7308) | Use format(): "-replayprotectionactivationtime={}".format(ACTIVATION_TIME) |
100 ↗ | (On Diff #7308) | Move the comment above |
172 ↗ | (On Diff #7308) | Not needed, already achieved by make_conform_to_ctor |
208 ↗ | (On Diff #7308) | self.nodes[0] is not a function |
305 ↗ | (On Diff #7308) | The test seems to be doing the right checks, but starting this point, I started having a hard time reviewing it.
|
This is in pretty good shape now. The test looks great. I still got a few things to improve, but this is getting there.
src/validation.cpp | ||
---|---|---|
2200 ↗ | (On Diff #7308) | I don' think GetBlockScriptFlags accepts null as an argument. |
2205 ↗ | (On Diff #7308) | You can keep the message here as this is effectively what we are doing at this point in time. The fact that things will be reprocessed later is independent of what we are doing here. |
2487 ↗ | (On Diff #7308) | dito, there is no reprocessing going on here. |
2764 ↗ | (On Diff #7308) | This is where the reprocessing is happening. If you think is is required, this is where the message should go. |
2205 ↗ | (On Diff #7291) | Thinking about it, I think it's fine to just say the mempool is cleared. That doesn't prevent to reinsert things at a later stage if appropriate :) |
test/functional/abc-mempool-coherence-on-activations.py | ||
31 ↗ | (On Diff #7308) | |
124 ↗ | (On Diff #7308) | If you mock time, it's better to go on based on this rather than use time(), which make tests non reproducible. |
305 ↗ | (On Diff #7308) | dito |
333 ↗ | (On Diff #7308) | Use a loop like at line 244. |
Gonna take control of this as florian will be away for the next few days, and this needs landing soon. Thanks for the hard work thus far @florian . :-D
address requests for change:
- misc nits
- logging messages altered
- test now fully mock-timed
- expanded comments about how transactions are tracked in invalidateblock and reorg events
address jason comments
- constify isEmpty
- in functional test, convert redundant mempool checks to comments
test/functional/abc-mempool-coherence-on-activations.py | ||
---|---|---|
35 ↗ | (On Diff #7317) | The very last nit, please use "-replayprotectionactivationtime={}".format(ACTIVATION_TIME). |