Page MenuHomePhabricator

Add importMempool
ClosedPublic

Authored by florian on Feb 8 2019, 19:52.

Details

Reviewers
deadalnix
markblundeberg
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC9cd4f98596e9: Add importMempool
Summary

importMempool moves in-mempool transactions into a disconnectPool queue to enable the reprocessing of current mempool entries

Test Plan

test_bitcoin -t mempool_tests

Diff Detail

Event Timeline

florian created this revision.Feb 8 2019, 19:52
Owners added a reviewer: Restricted Owners Package.Feb 8 2019, 19:52
Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 8 2019, 19:52
Herald added a subscriber: schancel. · View Herald Transcript
markblundeberg added inline comments.Feb 8 2019, 20:30
src/txmempool.cpp
1325 ↗(On Diff #7250)

Not sure about this comment ... it looks like the intended callsite will only have cs_main locked.

(If cs_main is locked I'm not sure if it's important to lock pool.cs, but good idea anyway)

src/txmempool.h
932 ↗(On Diff #7250)

unrelated, probably good to leave out for now

jasonbcox requested changes to this revision.Feb 8 2019, 21:07
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/test/mempool_tests.cpp
798 ↗(On Diff #7250)

ids -> expectedOrder to make it clear that this is being checked against

833 ↗(On Diff #7250)

This block should loop over some number of orderings that include already-in-order and out-of-order txs for vtx.

src/txmempool.h
932 ↗(On Diff #7250)

agreed. no need to include it until it's needed.

This revision now requires changes to proceed.Feb 8 2019, 21:07
florian updated this revision to Diff 7253.Feb 8 2019, 23:06
florian marked 4 inline comments as done.

Included test cases in different txn orders

deadalnix requested changes to this revision.Feb 8 2019, 23:28
deadalnix added inline comments.
src/test/mempool_tests.cpp
797 ↗(On Diff #7253)

CheckDisconnectPoolOrder would be more apt.

805 ↗(On Diff #7253)

I guess the first check should be that both size do match as you risk out of bound access here and this can pass if the disconnectPool just drops everything.

815 ↗(On Diff #7253)

Please define things at point of use.

818 ↗(On Diff #7253)

Just use int when bit manipulation are not needed and range is sufficient.

828 ↗(On Diff #7253)

Making this const is probably a good idea. It'll avoid mistakes. Defining named constants for 5 and 6 would make the code easier to read and follow and also easier to update in the future if one want to change them.

831 ↗(On Diff #7253)

dito

839 ↗(On Diff #7253)

I'm pretty sure you can instantiate the vector in place so the vector gets destroyed and memory freed immediately after use.

840 ↗(On Diff #7253)

You probably want to do the check within CheckDisconnectPoolSort . When the codes forces you to repeat two things together, and if you forget one it's incorrect, then it's a hint that they are actually not separate things. Keeping them separate will inevitably lead to complexity and errors.

853 ↗(On Diff #7253)

I understand the id here correspond to the if of the transaction ? If so, then it's be clearer to just get it from there. If not, then maybe creating some struct that describe the elements of a test case would make the test cases more readable/easily followed.

860 ↗(On Diff #7253)

Create the disconnectPool in the loop body so you don't need to do this kind of stuff.

src/txmempool.cpp
1331 ↗(On Diff #7253)

Declare at point of use. This makes things easier to follow. More generally, this is a wall of code. Adding a few empty lines that separate the different actions would make it more readable without changing much. for instance:

  • Copy the mempool content into the vector.
  • Clear the mempool
  • Create and set the ordered_mempool (bad name BTW, as it is not a mempool).
  • Use ordered_mempool to splice everything where it needs to be (BTW i had no idea one could do this on multimap, this is brilliant).
  • Clear the vector ?
  • Free memory in the pool if necessary.

You have all the components, it just a matter of shuffling bits around in logical blocks.

1338 ↗(On Diff #7253)

I there a way to use std::move here ? We'd avoid incrementd', all the ref count to then decrement them all on clear.

1351 ↗(On Diff #7253)

You can keep it if you want, but you don't need a local here

This revision now requires changes to proceed.Feb 8 2019, 23:28
florian updated this revision to Diff 7266.Feb 9 2019, 01:23
florian marked 12 inline comments as done.

Many small changes as requested.

florian added inline comments.Feb 9 2019, 01:30
src/test/mempool_tests.cpp
828 ↗(On Diff #7253)

Ok for 6. But I'd like to keep 5 around, otherwise I'd have to create dynamic code to instantiate the vtx vector for addForBlock and to addUnchecked txns into the mempool, which would add some complexity I'd like to avoid.

860 ↗(On Diff #7253)

disconnectPool has an assert in its destructor. I do need to clear() it.

src/txmempool.cpp
1338 ↗(On Diff #7253)

Yes, because both CTxMemPool and CTxMemPoolEntry don't have any destruction logic.

deadalnix requested changes to this revision.Feb 9 2019, 16:24
deadalnix added inline comments.
src/test/mempool_tests.cpp
11

Put that with userland includes. This is not a system library.

804

I'm not sure why you need this. You just checked it before. If you want to make sure the program stops, you can use BOOST_REQUIRE_EQUAL

830

constant are CAPITAL_SNAKE_CASE . I'm also pretty sure this is not about how long the test is.

833

There is essentially no way for me to look at these test cases and ensure they are correct in any reliable way.

Test cases usually comes in the form of inputs => expect results. This is just a bunch of numbers.

844

I don't think you need to call the constructor explicitly.

862

Remove

864

Remove

src/txmempool.cpp
1330

Put the comment on one line and let clang-format handle them. Put dot at the end of sentence.

1333

This is clearly another paragraph from what comes before. Paragraph are separated by an empty line.

1337

So this is returning by value anyways, so std::move does nothing.

1339

It doesn't looks like the pool is used after this, so holding the lock on the pool do not seems necessary.

This revision now requires changes to proceed.Feb 9 2019, 16:24
florian updated this revision to Diff 7276.Feb 9 2019, 21:38
florian marked 10 inline comments as done.

Applied requested changes. Refactored unit test for readability.

florian added inline comments.Feb 9 2019, 21:38
src/test/mempool_tests.cpp
864

disconnectPool has an assert in its destructor. I do need to clear() it.

deadalnix requested changes to this revision.Feb 10 2019, 02:25
deadalnix added inline comments.
src/txmempool.cpp
1335 ↗(On Diff #7276)

This is using the pool and you moved the lock just after.

This revision now requires changes to proceed.Feb 10 2019, 02:25
florian updated this revision to Diff 7278.Feb 10 2019, 02:33

Fixed pool access outside of crictical section

deadalnix accepted this revision.Feb 10 2019, 14:30
markblundeberg accepted this revision.Feb 10 2019, 17:17
jasonbcox accepted this revision.Feb 11 2019, 00:07
This revision is now accepted and ready to land.Feb 11 2019, 00:07
This revision was automatically updated to reflect the committed changes.