Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC9a0a03e35160: [consensus + avalanche] Move miner fund from consensus to policy

Summary

Moving the miner fund from consensus to policy makes it possible to do the following without a flag day upgrade:

Change fund address
Change fund amount
Add staking rewards

This iteration of the patch (initial feedback in D12806) implements the policy after ConnectBlock but before the UTXO updates are flushed because alternatives are higher risk:

Before ConnectTip/ConnectBlock - Parking a block before checking consensus rules would prevent a node from banning peers sending invalid blocks.
After ConnectTip - Rewinding the in-progress state changes is fragile and prone to complications while backporting.

After Wellington activates, the miner fund will no longer be enforced as a consensus rule. Blocks not including the miner fund will be parked and voted on by avalanche.

Depends on D13150 and D13207

Test Plan

ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

minerfund-policy-connecttip

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 22081
Build 43797: Build Diff	lint-circular-dependencies · build-clang · build-clang-tidy · build-diff · build-debug · build-without-wallet
Build 43796: arc lint + arc unit

Event Timeline

sdulfari created this revision.Feb 9 2023, 22:13

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 9 2023, 22:13

Harbormaster completed remote builds in B21970: Diff 37869.Feb 9 2023, 22:32

More tests and cleaned up implementation

sdulfari published this revision for review.Feb 21 2023, 21:56

sdulfari edited the summary of this revision. (Show Details)

sdulfari added a parent revision: D13150: [avalanche] Add regression testing for flip flopping votes on blocks.

Harbormaster completed remote builds in B22046: Diff 37972.Feb 21 2023, 22:03

We should check reconciliation via avalanche also using the block policy. This can (should ?) be in a new test file.

src/validation.cpp
2710 ↗	(On Diff #37972)	The loop structure looks OK but the message is lacking details. You should consider adding at least which policy failed, and eventually a debug message. Interestingly there already is a data structure for this: ValidationState
2714 ↗	(On Diff #37972)	There is already pindexNew->nStatus and the BlockValidationState, no need for another status output
3047–3063 ↗	(On Diff #37972)	And you don't need the fParked anymore

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

sdulfari added inline comments.Feb 23 2023, 19:00

src/validation.cpp
2710 ↗	(On Diff #37972)	I will fix this in followup
3047–3063 ↗	(On Diff #37972)	As discussed offline I don't want to mix consensus validation state with policy. Instead I will introduce a block policy validation state object but this will come as a followup diff.

Remove unnecessary fParked
Add new functional test to ensure miner fund address and amount can be changed by policy

add release note

Harbormaster completed remote builds in B22068: Diff 38016.Feb 23 2023, 19:19

Harbormaster completed remote builds in B22070: Diff 38018.Feb 23 2023, 19:26

Fabien added inline comments.Feb 24 2023, 08:34

src/validation.cpp
3224 ↗	(On Diff #38018)	I'm not a fan of this, because pindexMostWork is not a const pointer so there is no guarantee it didn't get nulled after the call to ABCS