Page MenuHomePhabricator

[consensus + avalanche] Move miner fund from consensus to policy
ClosedPublic

Authored by sdulfari on Feb 9 2023, 22:13.

Details

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 22070
Build 43775: Build Diffbuild-debug · lint-circular-dependencies · build-without-wallet · build-clang · build-diff · build-clang-tidy
Build 43774: arc lint + arc unit

Event Timeline

More tests and cleaned up implementation

Fabien requested changes to this revision.Feb 22 2023, 10:22
Fabien added a subscriber: Fabien.

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
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
src/validation.cpp
3224

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

Rebase and add null check

test/functional/abc_p2p_avalanche_policy_minerfund.py
140 ↗(On Diff #38035)
182 ↗(On Diff #38035)

That's not necessary

doc/release-notes.md
10 ↗(On Diff #38035)

See D13171 for the formatting

Fabien requested changes to this revision.Feb 27 2023, 09:07

This is missing an avalanche rejection test case

This revision now requires changes to proceed.Feb 27 2023, 09:07
  • Rebase and fixup release notes
  • Add miner fund rejection test case
Fabien requested changes to this revision.Mar 1 2023, 09:25
Fabien added inline comments.
test/functional/abc_p2p_avalanche_policy_minerfund.py
189 ↗(On Diff #38149)

you're missing the f

This revision now requires changes to proceed.Mar 1 2023, 09:25
This revision is now accepted and ready to land.Mar 2 2023, 09:07