Support for BIP 23 block proposal #1816

luke-jr commented at 2:57 AM on September 10, 2012: member

This would aide greatly in ensuring miners aren't messing up blocks, without the expense of losing 50 BTC.

BitcoinPullTester commented at 12:18 PM on September 10, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c077555c77dd1a58bedd6466a086c4f116fd0e57 for binaries and test log.

gavinandresen commented at 2:08 PM on September 10, 2012: contributor

Nested-three-deep reject(DoS(error(...))) with two different error strings seems kinda crazy. If I wasn't familiar with the history of how that came to be I'd be befuddled.

Could it be simplified to just: return reject(errorMessage, int nDoS=0) ?

... where reject prints errorMessage to debug.log and saves it in the block, and then does the DoS thing if nDoS > 0. Then returns false.

luke-jr commented at 1:40 AM on September 11, 2012: member

I'll flatten this later and collapse the 3 levels of function wrappers, just needed to get something working for Eligius...

BitcoinPullTester commented at 12:10 AM on September 12, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/8f976aaa862c933c41c153007d0ffe3093ff475b for binaries and test log.

in src/main.h:None in 8f976aaa86 outdated

 836 | @@ -837,6 +837,8 @@ class CBlock
 837 |      // Denial-of-service detection:
 838 |      mutable int nDoS;
 839 |      bool DoS(int nDoSIn, bool fIn) const { nDoS += nDoSIn; return fIn; }
 840 | +    mutable std::string strRejectReason;
 841 | +    bool reject(const std::string& strRejectReasonIn, bool fIn) const { strRejectReason = strRejectReasonIn; return fIn; }

sipa commented at 12:55 PM on September 21, 2012:

I really don't like CBlocks storing their own reject string, seems like a layer violation. Maybe not directly related to this change, as nDoS does the same.

I'd rather see a CValidationResult which stores such information, which is returned or pass-by-ref inside the block- and transaction validation functions. That's maybe out of scope for this change, though.

luke-jr commented at 12:24 AM on November 15, 2012: member

Rebased and implemented @sipa 's CValidationResult solution.

BitcoinPullTester commented at 10:57 PM on November 24, 2012: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/212089d306d351e63111e91a969e9da1b1920cbc for test log.

This pull does not merge cleanly onto current master

BitcoinPullTester commented at 6:43 PM on November 27, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/46888e6abca27dd6d2132aab7cd63f25363057c6 for binaries and test log.

in src/main.cpp:None in 999365d9ba outdated

2267 | +        CBlockIndex* pindexPrev = mapBlockIndex[pblock->hashPrevBlock];
2268 | +        CBlockIndex indexDummy(*pblock);
2269 | +        indexDummy.pprev = pindexPrev;
2270 | +        indexDummy.nHeight = pindexPrev->nHeight + 1;
2271 | +        CCoinsViewCache viewNew(*pcoinsTip, true);
2272 | +        return pblock->ConnectBlock(state, &indexDummy, viewNew, true);

luke-jr commented at 4:57 PM on January 31, 2013:

This seems to have a race condition - or maybe just a bug when proposing based on a not-the-most-recent prevblock.

luke-jr commented at 9:38 PM on February 1, 2013:

Fixed in current code (stale-prevblk check above)

in src/main.cpp:None in 61814c3953 outdated

1809 | @@ -1810,7 +1810,7 @@ bool SetBestChain(CValidationState &state, CBlockIndex* pindexNew)
1810 |          // an overestimation, as most will delete an existing entry or
1811 |          // overwrite one. Still, use a conservative safety factor of 2.
1812 |          if (!CheckDiskSpace(100 * 2 * 2 * pcoinsTip->GetCacheSize()))
1813 | -            return state.Error();
1814 | +            return state.Error("out of disk space");

Diapolo commented at 8:45 PM on February 1, 2013:

I thought about adding these, too :), nice you catched that.

in src/main.cpp:None in 61814c3953 outdated

2212 |      // Preliminary checks
2213 | -    if (!pblock->CheckBlock(state))
2214 | +    if (!pblock->CheckBlock(state, fCheckPOW))
2215 |          return error("ProcessBlock() : CheckBlock FAILED");
2216 |  
2217 | +    bool fHasPOW = fCheckPOW;

Diapolo commented at 8:56 PM on February 1, 2013:

Why do you assign fCheckPOW here? I know that works, but I would set it to false and use fCheckPOW in the following if-clause. So we have fCheckPOW for calling CheckProofOfWork(), which writes it's result to fHasPOW.

luke-jr commented at 9:29 PM on February 1, 2013:

CheckBlock calls CheckProofOfWork if fCheckPOW is true.

Diapolo commented at 10:00 PM on February 1, 2013:

I just wanted you to differentiate between fCheckPOW and fHasPOW, which you don't do with the current initialisation, that's all :).

luke-jr commented at 10:37 PM on February 1, 2013:

fCheckPOW is "should the function check the proof-of-work?" fHasPOW is "does the block in fact satisfy the proof-of-work?"

Diapolo commented at 10:46 PM on February 1, 2013:

I know, you just dont seem to understand, what I am trying to tell you ;-).

jgarzik commented at 5:08 PM on May 30, 2013: contributor

No objections to the code. Driving use case?

luke-jr commented at 5:17 PM on May 30, 2013: member

Ability to test block templates before putting the effort into mining them.

jgarzik commented at 5:26 PM on May 30, 2013: contributor

That's a use case. What size user constituency is driving this? Do multiple pools want this?

luke-jr commented at 6:06 PM on May 30, 2013: member

As far as I know, only Eligius and EclipseMC are actively using this today. Any pool using Eloipool for their poolserver would be able to immediately take advantage of it in the simplest case. It is also necessary for both a multiple-validating-node-implementation economy, and miner-chooses-his-own-transactions GBT-based pool mining.

sipa commented at 6:10 PM on May 30, 2013: member

@jgarzik I don't think that's a requirement (though certainly something to take into consideration).

I like the idea of such functionality, as it allows miners to validate their work against multiple implementations. Especially with alternative full node implementations becoming available, having something like this may be inevitable. Plus it's a good debugging tool for checking whether new (unreleased) versions can accept the best chain.

On the other hand, I don't like the evolution that may follow from this, where miners become required to validate against a dozen implementations that may or may not differ in validation rules... that has nothing to do with this PR though.

petertodd commented at 9:04 PM on May 31, 2013: contributor

@sipa A good example where the validation is extremely useful is a safety net for bitcoin changes that could potentially create invalid blocks. For instance in discussions with pools and miners something that comes up with implementing replace-by-fee and the child-pays-for-parent code I'm working on is the danger that there will be some kind of bug that leads to an invalid block. (let alone a delibrate exploit) Sure you can test all you want on testnet, but it's impossible to be 100% sure, and any orphan costs ~$3000USD; I've got one pool that wants to implement replace-by-fee that has said they are going to wait until it's been tested on Eligius first.

sipa commented at 8:29 AM on June 1, 2013: member

@petertodd Sure, I agree it's a very good way to debug and test potentially forking changes. I just don't like what it may lead to.

rebroad commented at 3:43 AM on June 2, 2013: contributor

@sipa I agree it is good to be wary of where this may lead to. Are you meaning to imply that leaving things as they are may be a better alternative to the proposed solution made by Luke?

DrHaribo commented at 9:56 PM on June 4, 2013: none

I would like to have block proposal functionality for BitMinter as well.

ghost commented at 12:35 PM on July 10, 2013: none

I'd very much like this for my new pool (currently in private testing), if another "driver" is needed :)

gavinandresen commented at 1:08 AM on October 21, 2013: contributor

Rebase needed.

luke-jr commented at 12:29 AM on October 25, 2013: member

Rebased.

laanwj commented at 2:40 PM on November 11, 2013: member

Should this be closed now that #3185 is in?

luke-jr commented at 7:39 PM on November 11, 2013: member

No, #3185 is entirely unrelated.

luke-jr commented at 3:35 PM on December 3, 2013: member

Rebased on top of #3185.

jgarzik commented at 6:55 PM on June 6, 2014: contributor

Seems nominally OK, but I'm not convinced the CValidationState stuff is correct for all cases.

in src/main.cpp:None in 6707a08edb outdated

2306 | @@ -2307,6 +2307,9 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CDiskBlockPos* dbp)
2307 |          }
2308 |      }
2309 |  
2310 | +    if (!fWriteToDisk)

laanwj commented at 9:04 AM on June 25, 2014:

Instead of adding a fWriteToDisk boolean argument, wouldn't it be more clear to split up the function in a checking and disk-writing part?

luke-jr commented at 1:09 PM on June 26, 2014:

Splitting up these means the new split functions would need to assume the mutex is held already (thus not get it themselves) to ensure the mutex isn't released between checks & writing - forcing the callers to hold the mutex instead. In which case IMO those new methods would best be private, and the current AcceptBlock interface (with fWriteToDisk added) exposed as a wrapper. Is that a change that still makes sense to you?

laanwj commented at 3:59 PM on August 4, 2014:

Sounds fair to me. Except for adding a fWriteToDisk boolean argument, which was my problem with this in the first place. The idea would be to have two functions, one that only checks and one that checks and writes to disk.

in src/main.cpp:None in 6707a08edb outdated

2374 | @@ -2372,7 +2375,7 @@ void PushGetBlocks(CNode* pnode, CBlockIndex* pindexBegin, uint256 hashEnd)
2375 |      pnode->PushMessage("getblocks", chainActive.GetLocator(pindexBegin), hashEnd);
2376 |  }
2377 |  
2378 | -bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp)
2379 | +bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp, bool fCheckPOW)

laanwj commented at 9:05 AM on June 25, 2014:

Same here. I'm not a fan of boolean arguments that change the functionality of a function to something completely different. Would prefer just having two functions and moving the common stuff to a third.

luke-jr commented at 12:50 PM on June 26, 2014: member

Notes to self: We shouldn't store proposed orphan blocks (Edit: Confirmed, this doesn't). Ensure nBits is itself checked (Edit: via AcceptBlock).

BitcoinPullTester commented at 6:01 PM on June 26, 2014: none

Automatic sanity-testing: FAILED MERGE, see http://jenkins.bluematt.me/pull-tester/p1816_f21746c89f7b38440c471ad95bbd0158460ad677/ for test log.

This pull does not merge cleanly onto current master This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

gmaxwell commented at 9:04 PM on July 14, 2014: contributor

Needs rebase.

laanwj added the label Block Generation on Jul 31, 2014

jgarzik commented at 3:48 PM on August 4, 2014: contributor

Sounds like the consensus on this is to merge, once rebased and [self-]notes addressed.

luke-jr force-pushed on Oct 9, 2014

jgarzik commented at 3:18 PM on October 13, 2014: contributor

ACK collection... @gmaxwell @sipa @laanwj @gavinandresen ?

Seems like the consensus is to merge, and the author rebased as requested.

luke-jr commented at 9:41 PM on October 13, 2014: member

@sipa pointed out that ProcessBlock no longer performs the full validation of the new block, which breaks detecting such failures for this.

luke-jr commented at 9:49 PM on October 13, 2014: member

For that issue, see also #3727 (comment) and #5083

luke-jr force-pushed on Oct 30, 2014

luke-jr commented at 2:58 AM on October 30, 2014: member

Rebased on top of #5106, #3727, and #5103

luke-jr force-pushed on Nov 4, 2014

luke-jr force-pushed on Nov 17, 2014

in src/main.cpp:None in 76590af5bd outdated

2429 | +bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex** ppindex)
2430 | +{
2431 | +    if (!ContextualCheckBlockHeader(block, state, ppindex))
2432 | +        return false;
2433 | +
2434 | +    if (!*ppindex)

laanwj commented at 12:46 PM on November 17, 2014:

Please check ppindex itself for NULL as well before this assignment, as was the case in the old code

in src/rpcmining.cpp:None in 76590af5bd outdated

 512 | @@ -462,6 +513,10 @@ Value getblocktemplate(const Array& params, bool fHelp)
 513 |      UpdateTime(pblock, pindexPrev);
 514 |      pblock->nNonce = 0;
 515 |  
 516 | +    static Array aCaps;

laanwj commented at 12:49 PM on November 17, 2014:

Making this Array static looks like premature optimization to me. I'd worry about multiple threads using it at the same time (resulting in race conditions). I'd prefer to just build it when needed.

sipa commented at 1:46 PM on November 17, 2014:

Agree.

luke-jr commented at 4:49 PM on November 17, 2014:

Any way we can just make this a static const Array somehow? That seems cleanest, but not sure if it's possible to do in C++ :/

sipa commented at 4:51 PM on November 17, 2014:

static const Array aCaps = boost::assign::list_of("proposal");

in src/main.h:None in 76590af5bd outdated

 464 |  bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW = true);
 465 |  bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
 466 |  
 467 | +// Context-dependent validity checks
 468 | +bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex **ppindex= NULL);
 469 | +bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex* const & pindexPrev);

laanwj commented at 12:55 PM on November 17, 2014:

What is the reason to pass a CBlockIndex* const & instead of just a const CBlockIndex * for pindexPrev? (same for TestBlockValidity below)

sipa commented at 1:12 PM on November 17, 2014:

I expect it's to minimize the differences in the code that's moving from the Accept* functions to the ContextualCheck* functions. But I agree it would be cleaner to just pass in a CBlockIndex*.

in src/main.cpp:None in 76590af5bd outdated

2405 | +    const int nHeight = pindexPrev->nHeight + 1;
2406 | +
2407 | +    // Check that all transactions are finalized
2408 | +    BOOST_FOREACH(const CTransaction& tx, block.vtx)
2409 | +        if (!IsFinalTx(tx, nHeight, block.GetBlockTime())) {
2410 | +            return state.DoS(10, error("AcceptBlock() : contains a non-final transaction"),

laanwj commented at 1:01 PM on November 17, 2014:

pindex->nStatus |= BLOCK_FAILED_VALID; was removed here - verified that it still happens:

This call to state.DoS (and the one below) sets state.mode = MODE_INVALID and state.corruptionPossible = false
After return in AcceptBlock, where this code comes from, the check state.IsInvalid() && !state.CorruptionPossible() is made, in which case pindex->nStatus |= BLOCK_FAILED_VALID

laanwj commented at 1:02 PM on November 17, 2014:

Nit: please use %s w/ __func__ here for the error, so that it shows the right function name instead of AcceptBlock() (same on line 2419)

sipa commented at 1:41 PM on November 17, 2014:

Indeed, I like moving those manual nStatus updates out.

in src/main.cpp:None in 76590af5bd outdated

2585 | +        return false;
2586 | +    if (!ContextualCheckBlock(block, state, pindexPrev))
2587 | +        return false;
2588 | +    if (!ConnectBlock(block, state, &indexDummy, viewNew, true))
2589 | +        return false;
2590 | +    if (!state.IsValid())

sipa commented at 1:11 PM on November 17, 2014:

This shouldn't be necessary (but won't hurt): if any CValidationState-updating function sets a non-valid state, it should return false already, so this could be replaced by an assert(state.IsValid());

in src/rpcmining.cpp:None in 76590af5bd outdated

 282 | @@ -283,6 +283,38 @@ Value prioritisetransaction(const Array& params, bool fHelp)
 283 |  }
 284 |  
 285 |  
 286 | +static bool DecodeHexBlk(CBlock& block, const std::string hexblock)

sipa commented at 1:15 PM on November 17, 2014:

Can this go in core_read/core_io?

laanwj commented at 1:21 PM on November 17, 2014: member

utACK apart from above nits - commithash b61b59f9ce680f6d514e377b5fe591f5a3f7c6e4 https://dev.visucore.com/bitcoin/acks/1816

in src/rpcmining.cpp:None in 76590af5bd outdated

 295 | +        return false;
 296 | +    }
 297 | +}
 298 | +
 299 | +// NOTE: Assumes a conclusive result; if result is inconclusive, it must be handled by caller
 300 | +static Value BIP22ValidationResult(const CValidationState state)

sipa commented at 1:30 PM on November 17, 2014:

Pass by reference?

luke-jr commented at 4:48 PM on November 17, 2014:

Does it matter for const?

sipa commented at 4:49 PM on November 17, 2014:

Avoiding a copy.

in src/main.cpp:None in 76590af5bd outdated

2576 | +    CBlockIndex indexDummy(block);
2577 | +    indexDummy.pprev = pindexPrev;
2578 | +    indexDummy.nHeight = pindexPrev->nHeight + 1;
2579 | +
2580 | +    // NOTE: CheckBlockHeader is called by CheckBlock
2581 | +    // NOTE: ContextualCheckBlockHeader only assigns ppindex, which we don't need/want

sipa commented at 1:44 PM on November 17, 2014:

Does it? It seems AcceptBlockHeader does...

EDIT: Ignore, I misread.

luke-jr force-pushed on Nov 17, 2014

luke-jr commented at 7:20 PM on November 17, 2014: member

Comments addressed and QA RPC tests added.

in qa/rpc-tests/getblocktemplate_proposals.py:None in 98974b5e95 outdated

 105 | +        txlist = list(bytearray(a2b_hex(a['data'])) for a in (tmpl['coinbasetxn'],) + tuple(tmpl['transactions']))
 106 | +
 107 | +        # Test 0: Capability advertised
 108 | +        assert('proposal' in tmpl['capabilities'])
 109 | +
 110 | +        # NOTE: This test currently FAILS

TheBlueMatt commented at 2:36 AM on November 18, 2014:

Does this mean its a blocker, or just future feature?

luke-jr commented at 2:43 AM on November 18, 2014:

It means regtest mode is failing to enforce height-in-coinbase rules. No reason to think it affects real-world use.

in src/main.cpp:None in 98974b5e95 outdated

2333 | @@ -2334,7 +2334,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
2334 |      return true;
2335 |  }
2336 |  
2337 | -bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex** ppindex)
2338 | +bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex** ppindex)

sipa commented at 9:53 AM on November 18, 2014:

Can you move the duplicate/prevfinding code to AcceptBlockHeader, and have this function just take a CBlockIndex* pointer (which cannot be NULL)? I would prefer it if this function didn't need cs_main.

sipa commented at 4:30 PM on November 18, 2014: member

@luke-jr I'm hacking a bit on this myself now (just so we don't do double work).

sipa commented at 4:56 PM on November 18, 2014: member

@luke-jr See #5302. Feel free to disagree, cherry-pick, or edit into your commits.

luke-jr force-pushed on Nov 18, 2014

in src/rpcmining.cpp:None in e25e31d32d outdated

 630 | -    catch (const std::exception &) {
 631 | +    if (!DecodeHexBlk(pblock, params[0].get_str()))
 632 |          throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
 633 | +
 634 | +    uint256 hash = block.GetHash();
 635 | +    if (mapBlockIndex.find(hash) != mapBlockIndex.end()) {

sipa commented at 6:55 PM on November 18, 2014:

mapBlockIndex.count(hash) != 0 ?

sipa commented at 6:57 PM on November 18, 2014:

Actually, just having the header does not mean we already have the block.

luke-jr force-pushed on Nov 18, 2014

Abstract context-dependent block checking from acceptance a48f2d6ddd

CreateNewBlock and miner_tests: Also check generated template is valid by CheckBlockHeader, ContextualCheckBlockHeader, CheckBlock, and ContextualCheckBlock 4ea1be7fb8

TestBlockValidity function for CBlock proposals (used by CreateNewBlock) df08a626e0

miner_tests: Disable checkpoints so they don't fail the subsidy-change test 132ea9b48f

Abstract DecodeHexBlk and BIP22ValidationResult functions out of submitblock 3dcbb9b6b4

luke-jr force-pushed on Nov 18, 2014

laanwj commented at 9:38 AM on November 19, 2014: member

Seems ready to merge now? Ready to ACK this now @gmaxwell @sipa @gavinandresen ?

in src/rpcmining.cpp:None in 720efbe986 outdated

 631 | +
 632 | +    uint256 hash = block.GetHash();
 633 | +    BlockMap::iterator mi = mapBlockIndex.find(hash);
 634 | +    if (mi != mapBlockIndex.end()) {
 635 | +        CBlockIndex *pindex = mi->second;
 636 | +        if (pindex->nStatus & BLOCK_VALID_MASK == BLOCK_VALID_SCRIPTS)

sipa commented at 11:16 PM on November 19, 2014:

nit: pindex->IsValid(BLOCK_VALID_SCRIPTS)

sipa commented at 11:41 PM on November 19, 2014: member

Untested ACK.

Implement BIP 23 Block Proposal 9765a50cbd

QA RPC tests: Add tests block block proposals bc6cb4177b

submitblock: Check for duplicate submissions explicitly 60755dbf76

luke-jr force-pushed on Nov 20, 2014

luke-jr commented at 12:18 AM on November 20, 2014: member

(Made the change @sipa suggested.)

CreateNewBlock: Stick height in coinbase so we pass template sanity check b867e409e5

luke-jr force-pushed on Nov 20, 2014

laanwj added this to the milestone 0.10.0 on Nov 20, 2014

in src/core_io.h:None in b867e409e5

  16 | @@ -16,6 +17,7 @@ class UniValue;
  17 |  // core_read.cpp
  18 |  extern CScript ParseScript(std::string s);
  19 |  extern bool DecodeHexTx(CTransaction& tx, const std::string& strHexTx);
  20 | +extern bool DecodeHexBlk(CBlock&, const std::string& strHexBlk);

Diapolo commented at 4:12 PM on November 20, 2014:

Just for completeness, every other function here has a variable (name) specified, while CBlock has not. In the definition you use block.

luke-jr commented at 4:15 PM on November 20, 2014:

Better not to add redundant content.

Diapolo commented at 4:16 PM on November 20, 2014:

You are such a contra guy sometimes ^^.

laanwj merged this on Nov 24, 2014

laanwj closed this on Nov 24, 2014

laanwj referenced this in commit f24bcce2ac on Nov 24, 2014

sdaftuar referenced this in commit 76b297c0d6 on May 14, 2015

KolbyML referenced this in commit 0fa40d7695 on Dec 5, 2020

KolbyML referenced this in commit 242356d012 on Dec 5, 2020

KolbyML referenced this in commit 7b2b9d048e on Dec 5, 2020

DrahtBot locked this on Sep 8, 2021