Page MenuHomePhabricator

Some simplification in the compact block code and tests
ClosedPublic

Authored by deadalnix on Jun 19 2017, 19:23.

Details

Summary

As per title.

Test Plan
make check
../qa/pull-tester/rpc-tests.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
compactsimplify
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 351
Build 351: arc lint + arc unit

Event Timeline

It looks ok to me, but I'd like another pair of eyes (or several) on this.

Ran all extended regtests excluding pruning, all passed.

src/net_processing.cpp
1630

This appears similarly redundant, also the if (!State(...)) before.

Furthermore, is it the right behavior to simply ignore a wrong-version compact block (as that appears to be what is happening here)?

src/net_processing.cpp
1622

This actually also appears to be redundant...

src/net_processing.cpp
1622

It is redundant with what ?

1630

Version 2 is segwit. We want to ignore.

Remove useless version check.

See comments, make check passes.

src/net_processing.cpp
1622

As I said in slack,

State(pfrom->GetId())->fProvidesHeaderAndIDs

is checked for being false and then

State(pfrom->GetId())->fProvidesHeaderAndIDs

is set to true, which appears redundant. fProvidesHeaderAndIDs is also a simple bool flag and not some complex object with an overloaded assignment operator.

When I asked you on this you said 'side effects, multithreading'. But if there is a multithreading side-effect problem here, shouldn't the if(..) part be locked somehow?

Anyways, do as you think makes the most sense.

1630

Ok, makes sense on the SegWit part. Otherwise, same comment as above.

src/net_processing.cpp
1622

In one case there is a write, in the other case there is no write. This is semantic change.

This revision is now accepted and ready to land.Jun 22 2017, 17:52

Accepted this on account of argument against semantic change.
If there was such a semantic change it would need separate testing to show it is ok.

This revision was automatically updated to reflect the committed changes.