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
test_runner.py abc-segwit-recovery-activation
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- segwit_recovery_activation
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4959 Build 7981: Bitcoin ABC Buildbot (legacy) Build 7980: arc lint + arc unit
Event Timeline
| 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) |
|
| 1214 ↗ | (On Diff #7221) | Remove |
| src/validation.cpp | ||
|---|---|---|
| 644 ↗ | (On Diff #7221) | IsGreatWallEnabledForCurrentBlock will end up getting called 100% of the time anyway when combined with schnorr activation. |
| 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:
- Moved complex test cases of mempool reorg handling to D2527
- Updated to follow the basic structure of abc-replay-protection
- Changed the testing mechanism of node banning behaviour
| src/validation.cpp | ||
|---|---|---|
| 644 ↗ | (On Diff #7284) | Placeholder for additional code from other diffs. |
| 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 ? |
| src/validation.cpp | ||
|---|---|---|
| 1209 ↗ | (On Diff #7284) | Discussed offline, this is good. maybe the comment could be made a bit clearer.
Something along these lines. Maybe a native speaker can give you something better. |
| 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. |
| 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. |
| test/functional/abc-segwit-recovery-activation.py | ||
|---|---|---|
| 234 | nit(s): # Move the mocktime for all nodes
for node in self.nodes:
node.setmocktime(GREAT_WALL_START_TIME) | |
| test/functional/abc-segwit-recovery-activation.py | ||
|---|---|---|
| 234 | 👍 will do | |
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) |
|
| 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. |
| 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. |