Page MenuHomePhabricator

Add an RPC to finalize a block
ClosedPublic

Authored by deadalnix on Nov 18 2018, 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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

deadalnix changed the visibility from "Public (No Login Required)" to "Restricted Project (Project)".Nov 18 2018, 01:03
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.Nov 18 2018, 01:13
deadalnix marked an inline comment as done.
deadalnix added inline comments.
test/functional/abc-finalize-block.py
76 ↗(On Diff #5866)

Ho yes, correct.

This revision is now accepted and ready to land.Nov 18 2018, 01:24
jasonbcox requested changes to this revision.Nov 18 2018, 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.Nov 18 2018, 01:33
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.

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.

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

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

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 added inline comments.
test/functional/abc-finalize-block.py
70 ↗(On Diff #5867)

Oh I see.

This revision is now accepted and ready to land.Nov 18 2018, 01:58
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 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)".Nov 20 2018, 16:29
This revision was automatically updated to reflect the committed changes.