Page MenuHomePhabricator

Add aserti3-2d support

Authored by deadalnix on Aug 8 2020, 11:56.


Group Reviewers
Restricted Project
rABC5b4c8ede6a9b: Add aserti3-2d support

As per title.

Test Plan
ninja check-pow

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

Upadte as per recent revision to the ASERTi3-2d spec

majcosta requested changes to this revision.Aug 9 2020, 21:26
majcosta added a subscriber: majcosta.
majcosta added inline comments.
216 ↗(On Diff #22845)

should probably const this

This revision now requires changes to proceed.Aug 9 2020, 21:26

I'll do another pass once the spec is nailed down in an accessible place.

191 ↗(On Diff #22846)

If though the assertion below makes this less risky, it doesn't hurt to make this const as well.

197 ↗(On Diff #22846)

nit: fix this comment formatting

202 ↗(On Diff #22846)

unpecified -> unspecified

Backport various updates.

  • caching the activation block does not require locking anymore.
  • add test for activation.

The test was modified as to not mutate Consensus::Params to change the activation of the DAA. Instead, ChainParamsWithDAAActivation was introduced.

jasonbcox requested changes to this revision.Aug 13 2020, 00:51
jasonbcox added inline comments.
468 ↗(On Diff #22875)

Use anchorBits0 everywhere this value is present. This makes it obvious that this value shouldn't be changed independently of the others.

477 ↗(On Diff #22875)

Ditto for anchorBits1

491 ↗(On Diff #22875)

These three BOOST_CHECK_EQUAL calls can be made into a helper function and used throughout the test. The test isn't consistent about the cache testing, and doing this will make the test much easier to read.

This revision now requires changes to proceed.Aug 13 2020, 00:51
deadalnix added inline comments.
468 ↗(On Diff #22875)

I don't think this solves any problem.

477 ↗(On Diff #22875)


491 ↗(On Diff #22875)

I don't think this is buying very much.

Other than some readability and mutability improvements in the tests, the rest looks good to me.
I did notice the spec is actually lacking in some regards (the implementation actually made it more clear than the pseudo-code) , so I made the appropriate notes there:

67 ↗(On Diff #22875)

const initialBits and dMaxErr

78 ↗(On Diff #22875)

If you have to comment the variable name, it's probably a bad name. Just call it bidx or something like you did in the other parts.

226 ↗(On Diff #22875)


275 ↗(On Diff #22875)


399 ↗(On Diff #22875)


468 ↗(On Diff #22875)

It does, because the alternative is to Ctrl+F for 0x180236e1 to make sure all of these values are the same, rather than wasting my time verifying that every jumble of characters matches.

Backport changes to CalculateASERT

Backport changes to the tests

majcosta added inline comments.
61 ↗(On Diff #22902)

testing for anchor->pskip is redundant, IsAxionEnabled already returns false if it's nullptr

210–213 ↗(On Diff #22902)

comments on their own line

This revision is now accepted and ready to land.Aug 15 2020, 13:02
jasonbcox retitled this revision from Add asserti3-2d support to Add aserti3-2d support.Aug 15 2020, 13:25
This revision was automatically updated to reflect the committed changes.