Add preciousblock RPC

sipa commented at 11:59 am on November 12, 2015: member

This adds a new PRC ‘preciousblock’ to mark a block as precious. A precious block will be treated as if it was received earlier than any other competing block.

laanwj added the label RPC on Nov 12, 2015

laanwj added the label Validation on Nov 12, 2015

sipa force-pushed on Nov 12, 2015

sipa commented at 1:38 pm on November 12, 2015: member

This was suggested in #6995.

kanoi commented at 3:32 pm on November 12, 2015: none

Awesome name :)

gmaxwell commented at 7:09 pm on November 12, 2015: contributor

Concept ACK.

This needs to document that overriding the first seen block behavior in slows network convergence and can increase the rate of long reorganizations.

in src/rpcblockchain.cpp: in 551151aff7 outdated

833+
834+    if (!state.IsValid()) {
835+        throw JSONRPCError(RPC_DATABASE_ERROR, state.GetRejectReason());
836+    }
837+
838+    return NullUniValue;

MarcoFalke commented at 8:26 pm on November 12, 2015:

Nit: What about being more verbose and return true? Considering the UX part of that, some users may interpret an empty line as “failed”.

Further reading: http://ux.stackexchange.com/a/12819

sipa commented at 8:35 pm on November 12, 2015:

It’s very common to return Null in case there is no real result in our API.

MarcoFalke commented at 8:43 pm on November 12, 2015:

Some places also use return true. But yes, null is more common.

MarcoFalke commented at 5:51 pm on November 17, 2015:

new issue: #7043

TheBlueMatt commented at 8:28 pm on November 12, 2015: member

I would prefer if it would only prefer the given block over other blocks that arrived within, eg, 10 seconds of the other block in question.

On November 12, 2015 3:59:35 AM PST, Pieter Wuille notifications@github.com wrote:

This adds a new PRC ‘preciousblock’ to mark a block as precious. A precious block will be treated as if it was received earlier than any other competing block. You can view, comment on, or merge this pull request online at:

#6996

– Commit Summary –

Add preciousblock RPC

– File Changes –

M qa/pull-tester/rpc-tests.py (1) A qa/rpc-tests/preciousblock.py (66) M src/chain.h (2) M src/main.cpp (15) M src/main.h (3) M src/rpcblockchain.cpp (38)

– Patch Links –

#6996.patch #6996.diff

Reply to this email directly or view it on GitHub: #6996

sipa commented at 8:32 pm on November 12, 2015: member

@TheBlueMatt That would be a much larger change, as we don’t track receive time.

TheBlueMatt commented at 8:34 pm on November 12, 2015: member

Indeed, not sure it’s worth it, but would prefer if we could :).

On November 12, 2015 12:33:08 PM PST, Pieter Wuille notifications@github.com wrote:

@TheBlueMatt That would be a much larger change, as we don’t track receive time.

Reply to this email directly or view it on GitHub: #6996 (comment)

MarcoFalke commented at 8:43 pm on November 12, 2015: member

utACK

gmaxwell commented at 8:06 pm on November 13, 2015: contributor

I’m sympathetic with matt’s ask; but I don’t think it’s worth it– it’ll get bypassed (which then puts parties back into a mode of modifying the consensus code), and it would imply that 10 seconds was “safe”– I have no idea if it is… the inherent preference for your own blocks by virtue of hearing the first already hurts convergence; so I think it would be fair to say that no amount additional is “safe”… but having a well constructed option is much safer than people trying to achieve this with invalidateblock.

kanoi commented at 10:21 pm on November 13, 2015: none

Well any numerical limitation would of course only require a few bytes of code change to circumvent so would seem to be somewhat a waste of time considering the extra work involved: as @sipa suggests

sipa commented at 0:58 am on November 28, 2015: member

@gmaxwell What do you mean by ‘saturate’ ?

sipa force-pushed on Nov 28, 2015

sipa commented at 1:37 am on November 28, 2015: member

Ok, changed the approach a bit. It now only keeps a counter per tip, and resets it when more work was added.

gmaxwell commented at 1:05 pm on November 28, 2015: contributor

By saturate I mean what you did, sat_add(int_min, -1) = int_min

sipa force-pushed on Nov 30, 2015

sipa commented at 12:41 pm on November 30, 2015: member

Rebased.

jgarzik commented at 5:00 pm on November 30, 2015: contributor

ut ACK

sipa commented at 5:05 pm on November 30, 2015: member

There is a bug in the unit tests that can lead to a race condition.

sipa force-pushed on Dec 1, 2015

sipa commented at 4:34 pm on December 1, 2015: member

Fixed.

sipa force-pushed on Dec 11, 2015

sipa commented at 8:32 pm on December 11, 2015: member

Rebased.

in qa/rpc-tests/preciousblock.py: in 6fda880823 outdated

32+        assert(self.nodes[1].getblockcount() == 3)
33+        hashG = self.nodes[1].getbestblockhash()
34+        assert(hashC != hashG)
35+        print "Connect nodes and check no reorg occurs"
36+        connect_nodes_bi(self.nodes,0,1)
37+        sync_blocks(self.nodes[0:2])

sdaftuar commented at 6:24 pm on December 14, 2015:

FYI, i don’t think this call to sync_blocks is doing anything helpful – it is just checking that getblockcount is the same between the two nodes, which it will be immediately, even before any headers or blocks are sent between the two nodes.

Perhaps this should sync on getchaintips() returning the expected output?

in qa/rpc-tests/preciousblock.py: in 6fda880823 outdated

42+        assert(self.nodes[0].getbestblockhash() == hashG)
43+        print "Make Node0 prefer block C again"
44+        self.nodes[0].preciousblock(hashC)
45+        assert(self.nodes[0].getbestblockhash() == hashC)
46+        print "Make Node1 prefer block C"
47+        self.nodes[1].preciousblock(hashC)

sdaftuar commented at 6:24 pm on December 14, 2015:

There’s a race condition here; it’s possible for nodes[1] to have not yet received hashC at the time this preciousblock call happens, which will cause the sync_chain in the next line to hang indefinitely.

(I had this happen to me on one of my runs of this test.)

laanwj commented at 1:48 pm on February 1, 2016: member

Needs rebase

in qa/rpc-tests/preciousblock.py: in 6fda880823 outdated

15+        print("Initializing test directory "+self.options.tmpdir)
16+        initialize_chain_clean(self.options.tmpdir, 3)
17+
18+    def setup_network(self):
19+        self.nodes = []
20+        self.is_network_split = False

luke-jr commented at 6:15 pm on February 12, 2016:

Extra space at the end of the line.

luke-jr referenced this in commit 1683eb9b80 on Feb 13, 2016

luke-jr commented at 3:00 pm on February 26, 2016: member

This can break CheckBlockIndex:

0if (pindex->nChainTx == 0) assert(pindex->nSequenceId == 0);  // nSequenceId can't be set for blocks that aren't linked

This assumption is no longer true if we use preciousblock on a not-yet-downloaded block (and this can occur in the RPC test).

gmaxwell commented at 7:06 am on April 19, 2016: contributor

Needs rebase.

in qa/rpc-tests/preciousblock.py: in 6fda880823 outdated

0@@ -0,0 +1,87 @@
1+#!/usr/bin/env python2

MarcoFalke commented at 7:24 pm on June 3, 2016:

Needs to be changed to python3 after rebase.

Also some other adjustments are needed in this file. (I am happy to do those)

btcdrak commented at 10:37 am on June 14, 2016: contributor

needs rebase

laanwj added the label Feature on Jun 16, 2016

luke-jr referenced this in commit cdffd18f1a on Jun 27, 2016

luke-jr referenced this in commit a29b237845 on Jun 27, 2016

luke-jr referenced this in commit bb7b476843 on Jun 28, 2016

luke-jr referenced this in commit e7a34570f7 on Aug 6, 2016

luke-jr commented at 9:28 pm on August 9, 2016: member

Pushed a rebase to my repo

laanwj commented at 10:09 am on August 19, 2016: member

Are you still planning to do this?

sipa commented at 12:10 pm on August 26, 2016: member

@luke-jr Your rebase does not fix the comment you made here: #6996 (comment) ?

sipa force-pushed on Aug 26, 2016

sipa commented at 2:09 pm on August 26, 2016: member

@luke-jr Switched to your commit e7a3457, and changed the test to python3 and updated the assert. The tests don’t run, though. I’ll inventigate later.

in qa/rpc-tests/preciousblock.py: in 70b1d511a2 outdated

 9+
10+from test_framework.test_framework import BitcoinTestFramework
11+from test_framework.util import *
12+
13+class PreciousTest(BitcoinTestFramework):
14+    def setup_chain(self):

MarcoFalke commented at 5:25 pm on August 26, 2016:

I think you need to use __init__() here, instead.

luke-jr commented at 8:36 pm on August 26, 2016: member

@sipa It is fixed in eafeb97f971e452720aeb645175384f660393660, and the tests updated in 5e6af8248f3980234e4aeb15f77ca61ed4124ef1 (both of which are part of my “preciousblock” branch)

Add preciousblock RPC

Includes a bugfix by Luke-Jr.

5127c4f21c

Add preciousblock tests

Rebased, improved and extended by Luke-Jr.

5805ac836c

sipa force-pushed on Aug 26, 2016

sipa commented at 9:07 pm on August 26, 2016: member

@luke-jr Ah, a link to that would have been useful :)

I’ve squashed the fixes and split into 2 commits.

laanwj commented at 7:36 pm on October 18, 2016: member

utACK 5805ac8

laanwj merged this on Oct 18, 2016

laanwj closed this on Oct 18, 2016

laanwj referenced this in commit 7f71a3c591 on Oct 18, 2016

btcdrak commented at 0:12 am on October 19, 2016: contributor

utACK 5805ac836c5847bc54cbef3e71154d022ca18eda

rebroad commented at 3:55 am on October 19, 2016: contributor

that was a surprise merge! Are there any plans that anyone knows of to use this? I would love to see some use-cases for documentation purposes.

codablock referenced this in commit 23bf8a6d46 on Sep 19, 2017

codablock referenced this in commit 6e871c0c2e on Jan 12, 2018

andvgal referenced this in commit 1ac35f1759 on Jan 6, 2019

MarcoFalke locked this on Sep 8, 2021

Add preciousblock RPC #6996