Page MenuHomePhabricator

Add activation code for SEGWIT_RECOVERY

Authored by markblundeberg on Feb 1 2019, 17:36.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
rSTAGINGf19955048697: Add activation code for SEGWIT_RECOVERY
rABCf19955048697: Add activation code for SEGWIT_RECOVERY

As per title.
Depends on D2468 and D2527.

Test Plan abc-segwit-recovery-activation

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 4903
Build 7869: Bitcoin ABC Buildbot (legacy)
Build 7868: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
deadalnix requested changes to this revision.Feb 7 2019, 23:40
deadalnix added inline comments.

It looks like a good algorithm. However, this doesn't lock the mempool and adds one more dependency on a global variable. It would be beneficial to pass the mempool as an argument, and make sure the locking is appropriate.

This also doesn't seems to clear the mempool.

I think this should probably be its own patch and come with a test.


Check that one first, as it is pretty much free.





This revision now requires changes to proceed.Feb 7 2019, 23:40
markblundeberg added inline comments.Feb 7 2019, 23:56

IsGreatWallEnabledForCurrentBlock will end up getting called 100% of the time anyway when combined with schnorr activation.

florian marked 10 inline comments as done.Feb 10 2019, 08:09
florian added inline comments.

Other activations will make that first conditional be tested every time.

florian updated this revision to Diff 7284.Feb 10 2019, 08:22

Moved mempool reorg handling to other diff, refactored the functional test

Mempool reorg handling was moved to D2527.

Refactored the functional test:

  1. Moved complex test cases of mempool reorg handling to D2527
  2. Updated to follow the basic structure of abc-replay-protection
  3. Changed the testing mechanism of node banning behaviour
florian added inline comments.Feb 10 2019, 12:43
644 ↗(On Diff #7284)

Placeholder for additional code from other diffs.

deadalnix requested changes to this revision.Feb 10 2019, 19:05
deadalnix added inline comments.
644 ↗(On Diff #7284)

If code needs to be added here, people can change the code and add it. There is no need to add extra complexity that is unrelated to this patch because someone may want to do something in the future.

As a rule of thumb, simpler code is more likely to be adaptable to whatever you want to do in the future then more complex code, so making code more complex so that it is extra adaptable is always almost a loss.

See J2EE, SOAP, CORBA, etc...

1209 ↗(On Diff #7284)

The variable is clearly named mandatoryFlags. When you write a comment that have to explain that the variable is not what it say it is, it is time to stop and reconsider.

1215 ↗(On Diff #7284)

You can add the flag here for instance.

50 ↗(On Diff #7284)

Isn't it the value by default ?

129 ↗(On Diff #7284)

These aren't the most descriptive names. There is an actual difference between these nodes.

151 ↗(On Diff #7284)

The name doesn't match what the comment say it does.

208 ↗(On Diff #7284)

It does sounds like this changes the TestManager's p2p_connections. Therefore TestManager_p2p_connections is a misnomer. Surely you are interested in keeping that value around for another reason than because it is Testmanager's p2p_connections.

At this point in the patch, this is very unclear to me why, when picking a proper name could have provided that information instantly.

Grepping in the test folder, I can't find any test that actually set that variable, ad generally setting the fields of another object that way raises flags. I'm pretty sure the other object's implementation really doesn't support this, and has no way to know one of its users is doing something sketchy, so this is bound to break in the future.

226 ↗(On Diff #7284)

You probably want to check for some peer specifically rather than the peer count. The would ensure that the peer sending the tx is the one you expect and so that the peer that isn't banned is the one you expect. For all you know, this could send the tx via the RPC and not test anything.

236 ↗(On Diff #7284)

Is that required ?

This revision now requires changes to proceed.Feb 10 2019, 19:05
deadalnix added inline comments.Feb 11 2019, 00:57
1209 ↗(On Diff #7284)

Discussed offline, this is good. maybe the comment could be made a bit clearer.

Even though it is not a mandatory flag, SCRIPT_ALLOW_SEGWIT_RECOVERY is strictly more permissive than the set of standard flags. It therefore needs to be added in order to check if we need to penalize the peer that sent us the transaction or not.

Something along these lines. Maybe a native speaker can give you something better.

Mengerian added inline comments.
1209 ↗(On Diff #7284)

As a native English speaker, what you wrote sounds good to me. I can't think of an obviously better wording.

florian marked 7 inline comments as done.Feb 11 2019, 17:51
florian added inline comments.
644 ↗(On Diff #7284)

Very well.

1209 ↗(On Diff #7284)


50 ↗(On Diff #7284)

Not in regtest! To avoid any doubt, I decided to explicitly set/unset this arg in both nodes.

129 ↗(On Diff #7284)


208 ↗(On Diff #7284)

Yeah, good call. I found an example I could use. I refactored this ban testing code to use only public interfaces.

236 ↗(On Diff #7284)

Yes, otherwise I get "time-too-new, block timestamp too far in the future (code 16)". And node1 (now node_std) needs to upgrade as well to test the banning behavior after the fork.

florian updated this revision to Diff 7299.Feb 11 2019, 17:55
florian marked 3 inline comments as done.

Refactored the ban testing code to only use public interfaces.

jasonbcox accepted this revision.Feb 11 2019, 22:47
jasonbcox added inline comments.
234 ↗(On Diff #7299)


# Move the mocktime for all nodes
for node in self.nodes:
markblundeberg commandeered this revision.Feb 13 2019, 16:35
markblundeberg added a reviewer: florian.

commandeering this one for the home stretch

rebase after D2527 landing

markblundeberg added inline comments.Feb 13 2019, 17:45
234 ↗(On Diff #7299)

👍 will do

deadalnix requested changes to this revision.Feb 13 2019, 19:07

Ok some comments, but nothing really bad. This will be in very soon.

644 ↗(On Diff #7323)

Fold the conditions and check the boolean first.

70 ↗(On Diff #7323)

No point in assigning to self.test if it is not used elsewhere (and it doesn't look like it is).

75 ↗(On Diff #7323)

I know many tests do it, but using time is really a bad idea as it make tests non reproducible.

200 ↗(On Diff #7323)

Because it is not expected that we will get banned, we can leave the timeout at the default value.

212 ↗(On Diff #7323)


234 ↗(On Diff #7323)

Because time is mocked, then it is easy to just remove all use of the time package and make the test reproducible.

276 ↗(On Diff #7323)

Maybe this should be check with the test line 263, because it's not obvious sending a txns to the node is done for both.

This revision now requires changes to proceed.Feb 13 2019, 19:07
markblundeberg marked 2 inline comments as done.Feb 13 2019, 21:23
markblundeberg marked 5 inline comments as done.
  • fully mock test time
  • one more sendrawtransaction rejection test
  • misc changes
markblundeberg marked an inline comment as done.Feb 13 2019, 21:50
markblundeberg added a subscriber: Fabien.

some tiny nits that @Fabien pointed out on D2483 / D2527

markblundeberg added inline comments.Feb 13 2019, 21:57
276 ↗(On Diff #7323)

not sure if I understood this comment right, but I added a new test next to 263 that *i think* you were suggesting.

deadalnix accepted this revision.Feb 13 2019, 22:38
This revision is now accepted and ready to land.Feb 13 2019, 22:38
This revision was automatically updated to reflect the committed changes.