Add preciousblock RPC #6996

Merged
merged 2 commits into from Oct 18, 2016

Projects

None yet
@sipa
Member
sipa commented Nov 12, 2015

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.

@sipa
Member
sipa commented Nov 12, 2015

This was suggested in #6995.

@kanoi
kanoi commented Nov 12, 2015

Awesome name :)

@gmaxwell
Member

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.

@MarcoFalke MarcoFalke and 1 other commented on an outdated diff Nov 12, 2015
src/rpcblockchain.cpp
+ {
+ LOCK(cs_main);
+ if (mapBlockIndex.count(hash) == 0)
+ throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
+
+ pblockindex = mapBlockIndex[hash];
+ }
+
+ CValidationState state;
+ PreciousBlock(state, Params().GetConsensus(), pblockindex);
+
+ if (!state.IsValid()) {
+ throw JSONRPCError(RPC_DATABASE_ERROR, state.GetRejectReason());
+ }
+
+ return NullUniValue;
@MarcoFalke
MarcoFalke Nov 12, 2015 Member

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
sipa Nov 12, 2015 Member

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

@MarcoFalke
MarcoFalke Nov 12, 2015 Member

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

@TheBlueMatt
Contributor

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 --

https://github.com/bitcoin/bitcoin/pull/6996.patch
https://github.com/bitcoin/bitcoin/pull/6996.diff


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

@sipa
Member
sipa commented Nov 12, 2015

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

@TheBlueMatt
Contributor

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
Member

utACK

@gmaxwell
Member

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
kanoi commented Nov 13, 2015

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
Member
sipa commented Nov 28, 2015

@gmaxwell What do you mean by 'saturate' ?

@sipa
Member
sipa commented Nov 28, 2015

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

@gmaxwell
Member

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

@sipa
Member
sipa commented Nov 30, 2015

Rebased.

@jgarzik
Member
jgarzik commented Nov 30, 2015

ut ACK

@sipa
Member
sipa commented Nov 30, 2015

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

@sipa
Member
sipa commented Dec 1, 2015

Fixed.

@sipa
Member
sipa commented Dec 11, 2015

Rebased.

@sdaftuar sdaftuar commented on an outdated diff Dec 14, 2015
qa/rpc-tests/preciousblock.py
+ self.nodes.append(start_node(1, self.options.tmpdir, ["-debug"]))
+ self.nodes.append(start_node(2, self.options.tmpdir, ["-debug"]))
+
+ def run_test(self):
+ print "Mine blocks A-B-C on Node 0"
+ self.nodes[0].generate(3)
+ assert(self.nodes[0].getblockcount() == 3)
+ hashC = self.nodes[0].getbestblockhash()
+ print "Mine competing blocks E-F-G on Node 1"
+ self.nodes[1].generate(3)
+ assert(self.nodes[1].getblockcount() == 3)
+ hashG = self.nodes[1].getbestblockhash()
+ assert(hashC != hashG)
+ print "Connect nodes and check no reorg occurs"
+ connect_nodes_bi(self.nodes,0,1)
+ sync_blocks(self.nodes[0:2])
@sdaftuar
sdaftuar Dec 14, 2015 Contributor

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?

@sdaftuar sdaftuar commented on an outdated diff Dec 14, 2015
qa/rpc-tests/preciousblock.py
+ assert(self.nodes[1].getblockcount() == 3)
+ hashG = self.nodes[1].getbestblockhash()
+ assert(hashC != hashG)
+ print "Connect nodes and check no reorg occurs"
+ connect_nodes_bi(self.nodes,0,1)
+ sync_blocks(self.nodes[0:2])
+ assert(self.nodes[0].getbestblockhash() == hashC)
+ assert(self.nodes[1].getbestblockhash() == hashG)
+ print "Make Node0 prefer block G"
+ self.nodes[0].preciousblock(hashG)
+ assert(self.nodes[0].getbestblockhash() == hashG)
+ print "Make Node0 prefer block C again"
+ self.nodes[0].preciousblock(hashC)
+ assert(self.nodes[0].getbestblockhash() == hashC)
+ print "Make Node1 prefer block C"
+ self.nodes[1].preciousblock(hashC)
@sdaftuar
sdaftuar Dec 14, 2015 Contributor

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
Member
laanwj commented Feb 1, 2016

Needs rebase

@luke-jr luke-jr commented on an outdated diff Feb 12, 2016
qa/rpc-tests/preciousblock.py
+
+#
+# Test PreciousBlock code
+#
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+class PreciousTest(BitcoinTestFramework):
+ def setup_chain(self):
+ print("Initializing test directory "+self.options.tmpdir)
+ initialize_chain_clean(self.options.tmpdir, 3)
+
+ def setup_network(self):
+ self.nodes = []
+ self.is_network_split = False
@luke-jr
luke-jr Feb 12, 2016 Member

Extra space at the end of the line.

@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@sipa @luke-jr sipa + luke-jr Add preciousblock RPC
Github-Pull: #6996
Rebased-From: 6fda880
1683eb9
@luke-jr
Member
luke-jr commented Feb 26, 2016

This can break CheckBlockIndex:

if (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
Member

Needs rebase.

@MarcoFalke MarcoFalke commented on an outdated diff Jun 3, 2016
qa/rpc-tests/preciousblock.py
@@ -0,0 +1,87 @@
+#!/usr/bin/env python2
@MarcoFalke
MarcoFalke Jun 3, 2016 edited Member

Needs to be changed to python3 after rebase.

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

@btcdrak
Member
btcdrak commented Jun 14, 2016

needs rebase

@laanwj laanwj added the Feature label Jun 16, 2016
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016
@luke-jr luke-jr Merge #6996 preciousblock-0.12.x bb7b476
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Aug 6, 2016
@sipa @luke-jr sipa + luke-jr Add preciousblock RPC
Github-Pull: #6996
Rebased-From: 6fda880
e7a3457
@luke-jr
Member
luke-jr commented Aug 9, 2016

Pushed a rebase to my repo

@laanwj
Member
laanwj commented Aug 19, 2016

Are you still planning to do this?

@sipa
Member
sipa commented Aug 26, 2016

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

@sipa
Member
sipa commented Aug 26, 2016

@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.

@MarcoFalke MarcoFalke commented on an outdated diff Aug 26, 2016
qa/rpc-tests/preciousblock.py
@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+# Copyright (c) 2015 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#
+# Test PreciousBlock code
+#
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import *
+
+class PreciousTest(BitcoinTestFramework):
+ def setup_chain(self):
@MarcoFalke
MarcoFalke Aug 26, 2016 Member

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

@luke-jr
Member
luke-jr commented Aug 26, 2016

@sipa It is fixed in eafeb97, and the tests updated in 5e6af82 (both of which are part of my "preciousblock" branch)

sipa added some commits Aug 26, 2016
@sipa sipa Add preciousblock RPC
Includes a bugfix by Luke-Jr.
5127c4f
@sipa sipa Add preciousblock tests
Rebased, improved and extended by Luke-Jr.
5805ac8
@sipa
Member
sipa commented Aug 26, 2016

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

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

@laanwj
Member
laanwj commented Oct 18, 2016

utACK 5805ac8

@laanwj laanwj merged commit 5805ac8 into bitcoin:master Oct 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Oct 18, 2016
@laanwj laanwj Merge #6996: Add preciousblock RPC
5805ac8 Add preciousblock tests (Pieter Wuille)
5127c4f Add preciousblock RPC (Pieter Wuille)
7f71a3c
@btcdrak
Member
btcdrak commented Oct 19, 2016

utACK 5805ac8

@rebroad
Contributor
rebroad commented Oct 19, 2016

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.

@jonasschnelli jonasschnelli referenced this pull request Oct 20, 2016
Open

TODO for release notes 0.14.0 #8455

1 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment