Page MenuHomePhabricator

Add activation code for SEGWIT_RECOVERY
ClosedPublic

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

Details

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

As per title.
Depends on D2468 and D2527.

Test Plan

test_runner.py abc-segwit-recovery-activation

Diff Detail

Repository
rABC Bitcoin ABC
Branch
cleanstack_exception
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4886
Build 7835: Bitcoin ABC Buildbot (legacy)
Build 7834: 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.
src/txmempool.cpp
1351 ↗(On Diff #7221)

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.

src/validation.cpp
644 ↗(On Diff #7221)

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

1208 ↗(On Diff #7221)

likestamp

1214 ↗(On Diff #7221)

Remove

This revision now requires changes to proceed.Feb 7 2019, 23:40
src/validation.cpp
644 ↗(On Diff #7221)

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.
src/validation.cpp
644 ↗(On Diff #7221)

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

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
src/validation.cpp
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.
src/validation.cpp
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.

test/functional/abc-segwit-recovery-activation.py
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
src/validation.cpp
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.
src/validation.cpp
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 added inline comments.
src/validation.cpp
644 ↗(On Diff #7284)

Very well.

1209 ↗(On Diff #7284)

Fixed!

test/functional/abc-segwit-recovery-activation.py
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)

Fixed.

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 marked 3 inline comments as done.

Refactored the ban testing code to only use public interfaces.

jasonbcox added inline comments.
test/functional/abc-segwit-recovery-activation.py
234 ↗(On Diff #7299)

nit(s):

# Move the mocktime for all nodes
for node in self.nodes:
    node.setmocktime(GREAT_WALL_START_TIME)
markblundeberg added a reviewer: florian.

commandeering this one for the home stretch

test/functional/abc-segwit-recovery-activation.py
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.

src/validation.cpp
644 ↗(On Diff #7323)

Fold the conditions and check the boolean first.

test/functional/abc-segwit-recovery-activation.py
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)

likestamp

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 5 inline comments as done.
  • fully mock test time
  • one more sendrawtransaction rejection test
  • misc changes
markblundeberg added a subscriber: Fabien.

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

test/functional/abc-segwit-recovery-activation.py
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.

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