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 387
Build 387: 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 ↗(On Diff #567)

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 ↗(On Diff #567)

This actually also appears to be redundant...

src/net_processing.cpp
1622 ↗(On Diff #567)

It is redundant with what ?

1630 ↗(On Diff #567)

Version 2 is segwit. We want to ignore.

Remove useless version check.

See comments, make check passes.

src/net_processing.cpp
1622 ↗(On Diff #567)

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 ↗(On Diff #567)

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

src/net_processing.cpp
1622 ↗(On Diff #567)

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.