Page MenuHomePhabricator

Improved mempool handling when changing validation rulesets
ClosedPublic

Authored by markblundeberg on Fri, Feb 8, 23:48.

Details

Reviewers
deadalnix
Fabien
florian
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC8ff468e92bf4: Improved mempool handling when changing validation rulesets
Summary

As per title.
Depends on D2525.

Test Plan

run abc-replay-protection.py and abc-mempool-coherence-on-activations.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix requested changes to this revision.Sat, Feb 9, 13:04
deadalnix added inline comments.
src/validation.cpp
2207 ↗(On Diff #7258)

Please install clang-format 7 and let it do the job.

2208 ↗(On Diff #7258)

This doesn't need to be in the else branch. No matter what, just clean the mempool. If it's already empty, the operation is essentially free.

2759 ↗(On Diff #7258)

Why is that part required ? This seems like it is not useful because updateMempoolForReorg will just do nothing if the pool is empty. This is just increase complexity for no benefit. unless I'm missing something.

This revision now requires changes to proceed.Sat, Feb 9, 13:04
markblundeberg added inline comments.Sat, Feb 9, 13:55
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.

deadalnix added inline comments.Sat, Feb 9, 16:57
test/functional/abc-replay-protection.py
9 ↗(On Diff #7267)

Don't have one thing do two things.

markblundeberg added inline comments.Sat, Feb 9, 18:33
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."

florian edited the test plan for this revision. (Show Details)Sun, Feb 10, 00:59
florian marked 5 inline comments as done.
florian updated this revision to Diff 7277.Sun, Feb 10, 01:02
florian edited the test plan for this revision. (Show Details)

Separated the functional test and fixed abc-replay-protection

deadalnix added inline comments.Sun, Feb 10, 18:23
src/validation.cpp
2203 ↗(On Diff #7277)

Use full sentences. Doing it that way makes things impossible to translate, for instance.

2761 ↗(On Diff #7277)

Sentences end with a dot.

florian marked an inline comment as done.Sun, Feb 10, 21:18
florian added inline comments.
src/validation.cpp
2203 ↗(On Diff #7277)

Indeed! Fixed.

florian updated this revision to Diff 7291.Sun, Feb 10, 21:19

Addressed comments.

Looks good on the functional test, you even did "Send post-fork-only txn into the mempool again" 👍

deadalnix requested changes to this revision.Sun, Feb 10, 21:43

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

This revision now requires changes to proceed.Sun, Feb 10, 21:43
florian updated this revision to Diff 7308.Tue, Feb 12, 02:22

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.

markblundeberg added inline comments.Tue, Feb 12, 02:57
test/functional/abc-mempool-coherence-on-activations.py
31 ↗(On Diff #7308)

Nice way to separate the concerns. :-)

Fabien requested changes to this revision.Tue, Feb 12, 15:59
Fabien added a subscriber: Fabien.
Fabien added inline comments.
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.
I found it really difficult to remember which transaction goes in which block, and jumped back and forth in the file all the time to figure it out. I would like to make some suggestions to make this easier to read:

  1. next_block() and update_block() both return the block. You can get the forkblockid and postforkblockid from there, rather than retrieving them by rewinding the chain through RPCs. This has the advantage that the variables are defined at the same place where the transactions goes into the blocks, keeping related things together.
  2. You can use some ascii-art in the comments to state which block contains which txns, or maybe just the mempool content with something like before [] =>after []. It is somewhat done in some comments but is not generalized.
This revision now requires changes to proceed.Tue, Feb 12, 15:59
deadalnix requested changes to this revision.Tue, Feb 12, 17:43

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)

likestamp

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.

markblundeberg commandeered this revision.Tue, Feb 12, 17:54
markblundeberg edited reviewers, added: florian; removed: markblundeberg.

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

markblundeberg marked 9 inline comments as done.Tue, Feb 12, 20:02
markblundeberg edited the test plan for this revision. (Show Details)

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
deadalnix accepted this revision.Tue, Feb 12, 23:10
jasonbcox requested changes to this revision.Tue, Feb 12, 23:25
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/txmempool.h
932 ↗(On Diff #7314)

bool isEmpty() const {

test/functional/abc-mempool-coherence-on-activations.py
317 ↗(On Diff #7314)

remove. you literally checked the mempool the line before this one

327 ↗(On Diff #7314)

remove; also duplicated above

This revision now requires changes to proceed.Tue, Feb 12, 23:25

address jason comments

  • constify isEmpty
  • in functional test, convert redundant mempool checks to comments
jasonbcox accepted this revision.Wed, Feb 13, 00:59
Fabien accepted this revision.Wed, Feb 13, 08:57
Fabien added inline comments.
test/functional/abc-mempool-coherence-on-activations.py
35 ↗(On Diff #7317)

The very last nit, please use "-replayprotectionactivationtime={}".format(ACTIVATION_TIME).
Sorry for the linter not being ready yet.

This revision is now accepted and ready to land.Wed, Feb 13, 08:57

last nit for % formatting

markblundeberg marked an inline comment as done.Wed, Feb 13, 15:05
Closed by commit rABC8ff468e92bf4: Improved mempool handling when changing validation rulesets (authored by florian, committed by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>). · Explain WhyWed, Feb 13, 15:23
This revision was automatically updated to reflect the committed changes.