As per title.
Details
Details
- Reviewers
majcosta jasonbcox - Group Reviewers
Restricted Project - Commits
- rABC5b4c8ede6a9b: Add aserti3-2d support
ninja check-pow
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| src/pow/aserti32d.cpp | ||
|---|---|---|
| 216 ↗ | (On Diff #22845) | should probably const this |
Comment Actions
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 |
Comment Actions
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.
| 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. |
Comment Actions
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. |