Page MenuHomePhabricator

Add an RPC to finalize a block
ClosedPublic

Authored by deadalnix on Sun, Nov 18, 01:03.

Details

Summary

A finalized block cannot be reorged. It effectively behaves very similarly to a checkpoint.

Based on work from @dagurval

Depends on D2057

Test Plan
./test/functional/test_runner.py abc-finalize-block

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Sun, Nov 18, 01:03
Herald added a reviewer: Restricted Project. · View Herald TranscriptSun, Nov 18, 01:03
deadalnix changed the visibility from "Public (No Login Required)" to "Restricted Project (Project)".Sun, Nov 18, 01:03
schancel accepted this revision.Sun, Nov 18, 01:13
schancel added a subscriber: schancel.
schancel added inline comments.
test/functional/abc-finalize-block.py
76 ↗(On Diff #5866)

Should this fail there doesn't seem to be able assertion here?

This revision is now accepted and ready to land.Sun, Nov 18, 01:13
deadalnix planned changes to this revision.Sun, Nov 18, 01:18
deadalnix marked an inline comment as done.
deadalnix added inline comments.
test/functional/abc-finalize-block.py
76 ↗(On Diff #5866)

Ho yes, correct.

deadalnix updated this revision to Diff 5867.Sun, Nov 18, 01:24

Fix test

This revision is now accepted and ready to land.Sun, Nov 18, 01:24
jasonbcox requested changes to this revision.Sun, Nov 18, 01:33
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/chain.cpp
189 ↗(On Diff #5866)

ancstor -> ancestor

src/rpc/blockchain.cpp
1515 ↗(On Diff #5866)

block -> this block otherwise it sounds ambiguous

1516 ↗(On Diff #5866)

Add to the end: USE WITH CAUTION!

1520 ↗(On Diff #5866)

mostpreciousblock -> finalizeblock

src/validation.cpp
2204 ↗(On Diff #5866)

This seems counter to the goal of the diff. DisconnectTip() is called from ActivateBestChainStep(), so doesn't that allow finalized blocks to be reorg'd?

This revision now requires changes to proceed.Sun, Nov 18, 01:33
deadalnix edited the summary of this revision. (Show Details)Sun, Nov 18, 01:40
deadalnix marked an inline comment as done.
deadalnix added a subscriber: dagurval.
deadalnix added inline comments.
src/validation.cpp
2204 ↗(On Diff #5866)

You can only do this via RPC. FindMostWorkChain will not pick that chain. Keeping the finalized block on an invalid chain effectively brick the node.

deadalnix updated this revision to Diff 5868.Sun, Nov 18, 01:42

Address comment

jasonbcox added inline comments.Sun, Nov 18, 01:45
src/validation.cpp
2405 ↗(On Diff #5867)

past -> prior to since "past" sounds like a block that is coming after the finalized block

3562 ↗(On Diff #5867)

The changes below this line can go in their own diff and be landed on master. Please do this to make this diff smaller and allow others to work on the code that is most improved.

test/functional/abc-finalize-block.py
70 ↗(On Diff #5867)

Shouldn't this log be up one line? the assert_equal has already checked that node and alt_node have the same chaintips.

jasonbcox added inline comments.Sun, Nov 18, 01:46
src/validation.cpp
2204 ↗(On Diff #5866)

Ok. Just double checking this was thought through. I see the check in FindMostWorkChain().

deadalnix marked an inline comment as done.Sun, Nov 18, 01:48
deadalnix added inline comments.
test/functional/abc-finalize-block.py
70 ↗(On Diff #5867)

And once we finalize, we reorg into the chain that is finalized.

jasonbcox marked an inline comment as done.Sun, Nov 18, 01:57
jasonbcox added inline comments.
test/functional/abc-finalize-block.py
70 ↗(On Diff #5867)

Oh I see.

jasonbcox accepted this revision.Sun, Nov 18, 01:58
This revision is now accepted and ready to land.Sun, Nov 18, 01:58
sickpig added a subscriber: sickpig.Sun, Nov 18, 15:11
sickpig added inline comments.
src/validation.cpp
2388 ↗(On Diff #5868)

Was it something we need to have done independently from the change introduced in this diff?

deadalnix marked an inline comment as done.Sun, Nov 18, 15:51
deadalnix added inline comments.
src/validation.cpp
2388 ↗(On Diff #5868)

Just checking existing invariants.

deadalnix changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".Tue, Nov 20, 16:29
This revision was automatically updated to reflect the committed changes.