Page MenuHomePhabricator

Add aserti3-2d support
ClosedPublic

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

Details

Reviewers
majcosta
jasonbcox
Group Reviewers
Restricted Project
Commits
rABC5b4c8ede6a9b: Add aserti3-2d support
Summary

As per title.

Test Plan
ninja check-pow

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
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.
src/pow/aserti32d.cpp
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.

src/pow/aserti32d.cpp
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.
src/pow/test/aserti32d_tests.cpp
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.
src/pow/test/aserti32d_tests.cpp
468 ↗(On Diff #22875)

I don't think this solves any problem.

477 ↗(On Diff #22875)

dito

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: https://github.com/bitcoincashorg/bitcoincash.org/pull/509#pullrequestreview-467735897

src/pow/test/aserti32d_tests.cpp
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)

const

275 ↗(On Diff #22875)

const

399 ↗(On Diff #22875)

const

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.
src/pow/aserti32d.cpp
61 ↗(On Diff #22902)

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

src/pow/test/aserti32d_tests.cpp
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.