Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABCa6a32a4fce4e: Introduce facility to evaluate miner fund as a block parking policy

Summary

This patch introduces MinerFundPolicy as a type of ParkingPolicy. These
policies can be called for a given block to determine if they pass various
checks. If not, the block can be parked and Avalanche voting will determine
its fate.

Unit tests are provided. Integration into validation code will come in a later
patch.

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

minerfund-policy-blockparking

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22039
Build 43713: Build Diff	lint-circular-dependencies · build-without-wallet · build-clang-tidy · build-diff · build-debug · build-clang
Build 43712: arc lint + arc unit

Event Timeline

sdulfari created this revision.Feb 14 2023, 22:50

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 14 2023, 22:50

sdulfari requested review of this revision.Feb 14 2023, 22:50

Harbormaster completed remote builds in B21987: Diff 37890.Feb 14 2023, 23:08

Fabien requested changes to this revision.Feb 15 2023, 09:55

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/policy/blockparking.cpp
15 ↗	(On Diff #37890)	see above, if pindex is null you got a segfault
19 ↗	(On Diff #37890)	That's not the right place for that. A better solution is to move this into the miner fund policy itself. The policy can even be moved to it's own file so the interface doesn't depend on activation code at all.
29 ↗	(On Diff #37890)	same here
src/policy/blockparking.h
23 ↗	(On Diff #37890)	I still don't like the negation here. I expect a policy to succeed if the block is compliant with that policy.
24 ↗	(On Diff #37890)	Just make it an interface
26 ↗	(On Diff #37890)	class, the members should be private
30 ↗	(On Diff #37890)	Can be null ?
41 ↗	(On Diff #37890)	why do you need a shared pointer ?
src/test/policy_blockparking_tests.cpp
41 ↗	(On Diff #37890)	need value ?
45 ↗	(On Diff #37890)	You can factor the code here by taking the script as an argument
110 ↗	(On Diff #37890)	You don't need the comment if the function was called GetBlockWithoutMinerFund
112 ↗	(On Diff #37890)	Related to the negation comment above, if you change the variable name from `shouldPark` to `policy` then the code makes absolutely no sense anymore.

This revision now requires changes to proceed.Feb 15 2023, 09:55

sdulfari added inline comments.Feb 16 2023, 18:52

src/policy/blockparking.h
24 ↗	(On Diff #37890)	I did attempt this the first time around. This time I could not repro the issue preventing me from doing so and I have forgotten the details. ¯\_(ツ)_/¯ It is fixed in the latest revision.
41 ↗	(On Diff #37890)	Got hung up here using unique_ptr because the initializer list in GetParkingPoliciesForBlock blows up because it makes a copy. The error message was not clear then but I tried again with a fresh mind and voila.

Address all feedback. Add more test cases and cleanup test code.

Harbormaster completed remote builds in B22006: Diff 37919.Feb 16 2023, 18:54

Build Bitcoin ABC Diffs / Diff Testing (lint-circular-dependencies) failed.

Tail of the build log:

/work /work/abc-ci-builds/lint-circular-dependencies
A new circular dependency in the form of "policy/blockparking/blockparking -> policy/blockparking/minerfund -> policy/blockparking/blockparking" appears to have been introduced.

/work/abc-ci-builds/lint-circular-dependencies
Build lint-circular-dependencies failed with exit code 1

Fabien added inline comments.Feb 16 2023, 19:53

src/policy/blockparking/blockparking.h
21 ↗	(On Diff #37919)	You don't need that anymore
28 ↗	(On Diff #37919)	This is probably not the right API. You'd better build the vector at callsite so you can have a different set of params for each policy as needed. All you need at the end is the interface. Also determining which policy to apply is the validation job (imagine you have some policy versioning to manage or something).

Improve file names. Remove now unneeded GetParkingPoliciesForBlock and other cleanup.

sdulfari edited the summary of this revision. (Show Details)Feb 16 2023, 22:33

Harbormaster completed remote builds in B22012: Diff 37925.Feb 16 2023, 22:41

Please add the test for genesis/first block also

src/policy/block/minerfund.cpp
12 ↗	(On Diff #37925)	That will crash during IBD/reindex as pindex/pindex.pprev are expected to be null. Should the policy pass on genesis block, and first block ? EDIT: I see pindex is no longer a pointer so can't be null. That means 2 things: the name is now bad, and this is inconsistent with all the other validation API
src/test/policy_block_tests.cpp
83 ↗	(On Diff #37925)	Nit: this is a repetitive block and can be optimized with a lambda
90 ↗	(On Diff #37925)	It took me infinite time to understand why this works as blockIndex is not updated, but SetMTP applies in place to the same blocks so the blocks.back() reference is updated with it.

This revision now requires changes to proceed.Feb 17 2023, 09:10

Fix leftover assumptions from previous iteration. Genesis block is now robustly supported.
Rename pindex -> blockIndex
Improve test coverage for early blocks including genesis
Refactor tests to make better use of lambdas and fix other minor issues throughout.

Harbormaster completed remote builds in B22016: Diff 37933.Feb 17 2023, 20:30

I left a couple suggestions for improvements but no blocker

src/policy/block/minerfund.cpp
12–19 ↗	(On Diff #37933)	if m_blockIndex.pprev is null you are sure wellington is not activated and the comment applies
src/test/policy_block_tests.cpp
79–82 ↗	(On Diff #37933)	The name implies the check Also the checkEarlyBlocks() can be part of checkMinerFundPolicy() and run every time.

This revision is now accepted and ready to land.Feb 18 2023, 10:27

Incorporate nits except moving checkEarlyBlocks. This would work as the test is now but it is not clear to me that this actually adds value.

Harbormaster completed remote builds in B22039: Diff 37959.Feb 20 2023, 21:54

Closed by commit rABCa6a32a4fce4e: Introduce facility to evaluate miner fund as a block parking policy (authored by sdulfari). · Explain WhyFeb 21 2023, 16:30

This revision was automatically updated to reflect the committed changes.

sdulfari added a commit: rABCa6a32a4fce4e: Introduce facility to evaluate miner fund as a block parking policy.

Introduce facility to evaluate miner fund as a block parking policy
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37959

src/CMakeLists.txt

src/policy/block/minerfund.h

src/policy/block/minerfund.cpp

src/policy/block/parkingpolicy.h

src/test/CMakeLists.txt

src/test/policy_block_tests.cpp

Introduce facility to evaluate miner fund as a block parking policyClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37959

src/CMakeLists.txt

src/policy/block/minerfund.h

src/policy/block/minerfund.cpp

src/policy/block/parkingpolicy.h

src/test/CMakeLists.txt

src/test/policy_block_tests.cpp

Introduce facility to evaluate miner fund as a block parking policy
ClosedPublic
Actions

Revision Contents
Changeset List