Support for BIP 23 block proposal #1816

luke-jr · 2012-09-10T02:57:58Z

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

BitcoinPullTester · 2012-09-10T12:18:49Z

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

gavinandresen · 2012-09-10T14:08:39Z

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 · 2012-09-11T01:40:14Z

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

BitcoinPullTester · 2012-09-12T00:10:44Z

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

sipa · 2012-09-21T12:55:09Z

src/main.h

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


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 · 2012-11-15T00:24:19Z

Rebased and implemented @sipa 's CValidationResult solution.

BitcoinPullTester · 2012-11-24T22:57:38Z

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 · 2012-11-27T18:43:57Z

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

luke-jr · 2013-01-31T16:57:44Z

src/main.cpp

+        indexDummy.pprev = pindexPrev;
+        indexDummy.nHeight = pindexPrev->nHeight   1;
+        CCoinsViewCache viewNew(*pcoinsTip, true);
+        return pblock->ConnectBlock(state, &indexDummy, viewNew, true);


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

Fixed in current code (stale-prevblk check above)

jgarzik · 2013-05-30T17:08:59Z

No objections to the code. Driving use case?

luke-jr · 2013-05-30T17:17:01Z

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

jgarzik · 2013-05-30T17:26:59Z

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

luke-jr · 2013-05-30T18:06:26Z

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 · 2013-05-30T18:10:32Z

@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 · 2013-05-31T21:04:24Z

@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 · 2013-06-01T08:29:29Z

@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 · 2013-06-02T03:43:46Z

@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 · 2013-06-04T21:56:00Z

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

ghost · 2013-07-10T12:35:32Z

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

gavinandresen · 2013-10-21T01:08:53Z

Rebase needed.

luke-jr · 2013-10-25T00:29:45Z

Rebased.

laanwj · 2013-11-11T14:40:07Z

Should this be closed now that #3185 is in?

luke-jr · 2013-11-11T19:39:26Z

No, #3185 is entirely unrelated.

luke-jr · 2013-12-03T15:35:52Z

Rebased on top of #3185.

jgarzik · 2014-06-06T18:55:32Z

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

laanwj · 2014-06-25T09:04:34Z

src/main.cpp

@@ -2307,6  2307,9 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CDiskBlockPos* dbp)
        }
    }

+    if (!fWriteToDisk)


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?

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?

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.

…d by CheckBlockHeader, ContextualCheckBlockHeader, CheckBlock, and ContextualCheckBlock

…e test

…itblock

laanwj · 2014-11-19T09:38:45Z

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

sipa · 2014-11-19T23:16:28Z

src/rpcmining.cpp

+    BlockMap::iterator mi = mapBlockIndex.find(hash);
+    if (mi != mapBlockIndex.end()) {
+        CBlockIndex *pindex = mi->second;
+        if (pindex->nStatus & BLOCK_VALID_MASK == BLOCK_VALID_SCRIPTS)


nit: pindex->IsValid(BLOCK_VALID_SCRIPTS)

sipa · 2014-11-19T23:41:11Z

Untested ACK.

luke-jr · 2014-11-20T00:18:11Z

(Made the change @sipa suggested.)

…heck

Diapolo · 2014-11-20T16:12:49Z

src/core_io.h

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


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

Better not to add redundant content.

You are such a contra guy sometimes ^^.

b867e40 CreateNewBlock: Stick height in coinbase so we pass template sanity check (Luke Dashjr) 60755db submitblock: Check for duplicate submissions explicitly (Luke Dashjr) bc6cb41 QA RPC tests: Add tests block block proposals (Luke Dashjr) 9765a50 Implement BIP 23 Block Proposal (Luke Dashjr) 3dcbb9b Abstract DecodeHexBlk and BIP22ValidationResult functions out of submitblock (Luke Dashjr) 132ea9b miner_tests: Disable checkpoints so they don't fail the subsidy-change test (Luke Dashjr) df08a62 TestBlockValidity function for CBlock proposals (used by CreateNewBlock) (Luke Dashjr) 4ea1be7 CreateNewBlock and miner_tests: Also check generated template is valid by CheckBlockHeader, ContextualCheckBlockHeader, CheckBlock, and ContextualCheckBlock (Luke Dashjr) a48f2d6 Abstract context-dependent block checking from acceptance (Luke Dashjr)

…1816 Conflicts: src/main.cpp

…further clean up. 063401c PoS miner: add height to the first input scriptSig of the coinstake tx remove unneded IncrementExtraNonce call in PoS blocks. (furszy) 7fc4680 Do not calculate the block value twice for the same block. (furszy) 7893818 Removing not needed CBlock::payee member. (furszy) 3e07280 Remove unused zPIVStake argument in FillBlockPayee. (furszy) f702628 miner: decouple coinbase transaction creation into its own function. (furszy) 377f87e FillBlockPayee: reworked to not require chainActive.Tip redundant access. (furszy) 920652b miner: remove FillBlockPayee unneded fee argument refactor coinbase tx creation to be in one single place and not dispersed across the CreateNewBlock method. (furszy) Pull request description: Another step forward improving the miner `CreateNewBlock` function. This time, have unified the coinbase tx creation flow that was previously dispersed over the function and having some redundant initializations/sets. Plus, cleaned up some unused and/or unneeded fields and function arguments. This is a preparation for bitcoin#1815 work. It will not be possible to modify any of the elements of a transaction once them are appended to a block, thus why the transaction needs (more than ever) to be crafted in one single place and not all over the sources. ACKs for top commit: Fuzzbawls: ACK 063401c random-zebra: utACK 063401c and merging... Tree-SHA512: c32cd87f82d9b31dedf9d47063a67978ef683bf35ff625823d6ece1a2d2904599dcbf15791682ffc1deb412082b1ecc24d11b364e300d4ceb259c65e65b0e9f6

…y once. 261bd22 Staking process: calculate the stakeable utxo only once. (furszy) Pull request description: Work on top of bitcoin#1816 and part of bitcoin#1817 coinstake transaction creation speedup goal. Starting on 01814d5 Improving the following situation: we are calculating the stakeable utxo twice for the same PoS block creation process (looping over the entire wallet's transactions map twice), one before calling `CreateNewBlock` in `CheckForCoins` and the second one inside the `CreateNewBlock` method when we call `CWallet::CreateCoinStake`. This PR fixes it adding an unique available coins vector calculated before the block creation, only once per try, in the `CheckForCoins` function and feeding `CreateNewBlock` with it. ACKs for top commit: random-zebra: utACK 261bd22 Tree-SHA512: f553667fd48d0d7eb78e2ce87b438c915dc216b32e0426e0214caa52f16a2627143319233c6dd93e4b5d45dcfbb825787c1b39cd209c8e6516bf2391f0516e00

911e31c Make CBlock::vtx a vector of shared_ptr<CTransaction> (furszy) 9d27b75 Add deserializing constructors to CTransaction and CMutableTransaction (Pieter Wuille) 5f90940 Add serialization for unique_ptr and shared_ptr (Pieter Wuille) Pull request description: Inspired on bitcoin#9125 (with many more adaptations), preparation for bitcoin#8580. Ant work 🐜 Establishing the bases for the work, we will need to continue migrating `CTransaction` to `std::shared_ptr<const CTransaction>` where is possible. Example bitcoin#8126 (can find more in bitcoin#1726 list). TODO: * back port final `CTransactionRef` implementation commit. EDIT: This is now depending on bitcoin#1816 . ACKs for top commit: random-zebra: ACK 911e31c Fuzzbawls: ACK 911e31c Tree-SHA512: 61d937aba7dce4ac0867496e7f81ae8351dcdd60b4e72c4f0ed58152a7e556bf455836c766bc97bbca331227e5deed92fa5ce609fa63bb9cb71600b430dc4405

sipa reviewed Sep 21, 2012
View reviewed changes

luke-jr mentioned this pull request Jan 27, 2013

Improve error handling during validation #2224

Merged

luke-jr reviewed Jan 31, 2013
View reviewed changes

laanwj reviewed Jun 25, 2014
View reviewed changes

luke-jr force-pushed the blkproposal branch 3 times, most recently from d098487 to c39a58c Compare November 18, 2014 19:13

luke-jr added 5 commits November 18, 2014 19:20

Abstract context-dependent block checking from acceptance

a48f2d6

CreateNewBlock and miner_tests: Also check generated template is vali…

4ea1be7

…d by CheckBlockHeader, ContextualCheckBlockHeader, CheckBlock, and ContextualCheckBlock

TestBlockValidity function for CBlock proposals (used by CreateNewBlock)

df08a62

miner_tests: Disable checkpoints so they don't fail the subsidy-chang…

132ea9b

…e test

Abstract DecodeHexBlk and BIP22ValidationResult functions out of subm…

3dcbb9b

…itblock

luke-jr force-pushed the blkproposal branch 2 times, most recently from 6c55698 to 720efbe Compare November 18, 2014 19:28

sipa reviewed Nov 19, 2014
View reviewed changes

luke-jr added 3 commits November 20, 2014 00:15

Implement BIP 23 Block Proposal

9765a50

QA RPC tests: Add tests block block proposals

bc6cb41

submitblock: Check for duplicate submissions explicitly

60755db

luke-jr force-pushed the blkproposal branch from 720efbe to 60755db Compare November 20, 2014 00:15

CreateNewBlock: Stick height in coinbase so we pass template sanity c…

b867e40

…heck

luke-jr force-pushed the blkproposal branch from 5f21923 to b867e40 Compare November 20, 2014 15:33

laanwj added this to the 0.10.0 milestone Nov 20, 2014

Diapolo reviewed Nov 20, 2014
View reviewed changes

laanwj merged commit b867e40 into bitcoin:master Nov 24, 2014

fanquake mentioned this pull request Feb 2, 2015

Add list of implemented BIPs #5708

Merged

sdaftuar added a commit to sdaftuar/bitcoin that referenced this pull request May 14, 2015

Merge 'upstream/master' and fix main.cpp merge conflict with bitcoin#…

76b297c

…1816 Conflicts: src/main.cpp

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Support for BIP 23 block proposal #1816

Support for BIP 23 block proposal #1816

Conversation

luke-jr commented Sep 10, 2012

BitcoinPullTester commented Sep 10, 2012

gavinandresen commented Sep 10, 2012

luke-jr commented Sep 11, 2012

BitcoinPullTester commented Sep 12, 2012

Choose a reason for hiding this comment

luke-jr commented Nov 15, 2012

BitcoinPullTester commented Nov 24, 2012

BitcoinPullTester commented Nov 27, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgarzik commented May 30, 2013

luke-jr commented May 30, 2013

jgarzik commented May 30, 2013

luke-jr commented May 30, 2013

sipa commented May 30, 2013

petertodd commented May 31, 2013

sipa commented Jun 1, 2013

rebroad commented Jun 2, 2013

DrHaribo commented Jun 4, 2013

ghost commented Jul 10, 2013

gavinandresen commented Oct 21, 2013

luke-jr commented Oct 25, 2013

laanwj commented Nov 11, 2013

luke-jr commented Nov 11, 2013

luke-jr commented Dec 3, 2013

jgarzik commented Jun 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Nov 19, 2014

Choose a reason for hiding this comment

sipa commented Nov 19, 2014

luke-jr commented Nov 20, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment