Skip to content

Support for BIP 23 block proposal #1816

Merged
merged 9 commits into from Nov 24, 2014
@luke-jr
Bitcoin member
luke-jr commented Sep 10, 2012

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

@BitcoinPullTester

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

@gavinandresen
Bitcoin member

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
Bitcoin member
luke-jr commented Sep 11, 2012

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

@BitcoinPullTester

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

@sipa sipa commented on an outdated diff Sep 21, 2012
@@ -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; }
@sipa
Bitcoin member
sipa added a note Sep 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@luke-jr
Bitcoin member
luke-jr commented Nov 15, 2012

Rebased and implemented @sipa 's CValidationResult solution.

@BitcoinPullTester

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

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

@luke-jr luke-jr commented on an outdated diff Jan 31, 2013
src/main.cpp
return error("ProcessBlock() : AcceptBlock FAILED");
+ if (!fHasPOW)
+ {
+ // The block isn't committed to disk since it was just a proposal, but we need to do connect checks still
+ CBlockIndex* pindexPrev = mapBlockIndex[pblock->hashPrevBlock];
+ CBlockIndex indexDummy(*pblock);
+ indexDummy.pprev = pindexPrev;
+ indexDummy.nHeight = pindexPrev->nHeight + 1;
+ CCoinsViewCache viewNew(*pcoinsTip, true);
+ return pblock->ConnectBlock(state, &indexDummy, viewNew, true);
@luke-jr
Bitcoin member
luke-jr added a note Jan 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
Bitcoin member
luke-jr added a note Feb 1, 2013

Fixed in current code (stale-prevblk check above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo Diapolo commented on an outdated diff Feb 1, 2013
@@ -1810,7 +1810,7 @@ bool SetBestChain(CValidationState &state, CBlockIndex* pindexNew)
// an overestimation, as most will delete an existing entry or
// overwrite one. Still, use a conservative safety factor of 2.
if (!CheckDiskSpace(100 * 2 * 2 * pcoinsTip->GetCacheSize()))
- return state.Error();
+ return state.Error("out of disk space");
@Diapolo
Diapolo added a note Feb 1, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Diapolo Diapolo and 1 other commented on an outdated diff Feb 1, 2013
return error("ProcessBlock() : CheckBlock FAILED");
+ bool fHasPOW = fCheckPOW;
@Diapolo
Diapolo added a note Feb 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
Bitcoin member
luke-jr added a note Feb 1, 2013

CheckBlock calls CheckProofOfWork if fCheckPOW is true.

@Diapolo
Diapolo added a note Feb 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
Bitcoin member
luke-jr added a note Feb 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
Diapolo added a note Feb 1, 2013

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jgarzik
Bitcoin member
jgarzik commented May 30, 2013

No objections to the code. Driving use case?

@luke-jr
Bitcoin member
luke-jr commented May 30, 2013

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

@jgarzik
Bitcoin member
jgarzik commented May 30, 2013

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

@luke-jr
Bitcoin member
luke-jr commented May 30, 2013

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
Bitcoin member
sipa commented May 30, 2013

@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
Bitcoin member

@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
Bitcoin member
sipa commented Jun 1, 2013

@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
rebroad commented Jun 2, 2013

@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
DrHaribo commented Jun 4, 2013

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

@hrabbach

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

@gavinandresen
Bitcoin member

Rebase needed.

@luke-jr
Bitcoin member
luke-jr commented Oct 25, 2013

Rebased.

@laanwj
Bitcoin member
laanwj commented Nov 11, 2013

Should this be closed now that #3185 is in?

@luke-jr
Bitcoin member
luke-jr commented Nov 11, 2013

No, #3185 is entirely unrelated.

@luke-jr
Bitcoin member
luke-jr commented Dec 3, 2013

Rebased on top of #3185.

@jgarzik
Bitcoin member
jgarzik commented Jun 6, 2014

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

@laanwj laanwj and 1 other commented on an outdated diff Jun 25, 2014
@@ -2307,6 +2307,9 @@ bool AcceptBlock(CBlock& block, CValidationState& state, CDiskBlockPos* dbp)
}
}
+ if (!fWriteToDisk)
@laanwj
Bitcoin member
laanwj added a note Jun 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
Bitcoin member
luke-jr added a note Jun 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
Bitcoin member
laanwj added a note Aug 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj commented on an outdated diff Jun 25, 2014
@@ -2372,7 +2375,7 @@ void PushGetBlocks(CNode* pnode, CBlockIndex* pindexBegin, uint256 hashEnd)
pnode->PushMessage("getblocks", chainActive.GetLocator(pindexBegin), hashEnd);
}
-bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp)
+bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp, bool fCheckPOW)
@laanwj
Bitcoin member
laanwj added a note Jun 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@luke-jr
Bitcoin member
luke-jr commented Jun 26, 2014

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

@BitcoinPullTester

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

Needs rebase.

@jgarzik
Bitcoin member
jgarzik commented Aug 4, 2014

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

@jgarzik
Bitcoin member
jgarzik commented Oct 13, 2014

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

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

@luke-jr
Bitcoin member
luke-jr commented Oct 13, 2014

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

@luke-jr
Bitcoin member
luke-jr commented Oct 13, 2014

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

@luke-jr
Bitcoin member
luke-jr commented Oct 30, 2014

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

@laanwj laanwj commented on an outdated diff Nov 17, 2014
+ CScript expect = CScript() << nHeight;
+ if (block.vtx[0].vin[0].scriptSig.size() < expect.size() ||
+ !std::equal(expect.begin(), expect.end(), block.vtx[0].vin[0].scriptSig.begin())) {
+ return state.DoS(100, error("AcceptBlock() : block height mismatch in coinbase"), REJECT_INVALID, "bad-cb-height");
+ }
+ }
+
+ return true;
+}
+
+bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex** ppindex)
+{
+ if (!ContextualCheckBlockHeader(block, state, ppindex))
+ return false;
+
+ if (!*ppindex)
@laanwj
Bitcoin member
laanwj added a note Nov 17, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj and 2 others commented on an outdated diff Nov 17, 2014
src/rpcmining.cpp
@@ -462,6 +513,10 @@ Value getblocktemplate(const Array& params, bool fHelp)
UpdateTime(pblock, pindexPrev);
pblock->nNonce = 0;
+ static Array aCaps;
@laanwj
Bitcoin member
laanwj added a note Nov 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
Bitcoin member
sipa added a note Nov 17, 2014

Agree.

@luke-jr
Bitcoin member
luke-jr added a note Nov 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
Bitcoin member
sipa added a note Nov 17, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj and 1 other commented on an outdated diff Nov 17, 2014
// Context-independent validity checks
bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, bool fCheckPOW = true);
bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
+// Context-dependent validity checks
+bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex **ppindex= NULL);
+bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex* const & pindexPrev);
@laanwj
Bitcoin member
laanwj added a note Nov 17, 2014

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

@sipa
Bitcoin member
sipa added a note Nov 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*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj and 1 other commented on an outdated diff Nov 17, 2014
if (ppindex)
*ppindex = pindex;
return true;
}
+bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex* const & pindexPrev)
+{
+ const int nHeight = pindexPrev->nHeight + 1;
+
+ // Check that all transactions are finalized
+ BOOST_FOREACH(const CTransaction& tx, block.vtx)
+ if (!IsFinalTx(tx, nHeight, block.GetBlockTime())) {
+ return state.DoS(10, error("AcceptBlock() : contains a non-final transaction"),
@laanwj
Bitcoin member
laanwj added a note Nov 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
Bitcoin member
laanwj added a note Nov 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
Bitcoin member
sipa added a note Nov 17, 2014

Indeed, I like moving those manual nStatus updates out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa sipa commented on an outdated diff Nov 17, 2014
+ CCoinsViewCache viewNew(pcoinsTip);
+ CBlockIndex indexDummy(block);
+ indexDummy.pprev = pindexPrev;
+ indexDummy.nHeight = pindexPrev->nHeight + 1;
+
+ // NOTE: CheckBlockHeader is called by CheckBlock
+ // NOTE: ContextualCheckBlockHeader only assigns ppindex, which we don't need/want
+ if (!ContextualCheckBlockHeader(block, state, NULL))
+ return false;
+ if (!CheckBlock(block, state, fCheckPOW, fCheckMerkleRoot))
+ return false;
+ if (!ContextualCheckBlock(block, state, pindexPrev))
+ return false;
+ if (!ConnectBlock(block, state, &indexDummy, viewNew, true))
+ return false;
+ if (!state.IsValid())
@sipa
Bitcoin member
sipa added a note Nov 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());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa sipa commented on an outdated diff Nov 17, 2014
src/rpcmining.cpp
@@ -283,6 +283,38 @@ Value prioritisetransaction(const Array& params, bool fHelp)
}
+static bool DecodeHexBlk(CBlock& block, const std::string hexblock)
@sipa
Bitcoin member
sipa added a note Nov 17, 2014

Can this go in core_read/core_io?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj
Bitcoin member
laanwj commented Nov 17, 2014

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

@sipa sipa and 1 other commented on an outdated diff Nov 17, 2014
src/rpcmining.cpp
@@ -283,6 +283,38 @@ Value prioritisetransaction(const Array& params, bool fHelp)
}
+static bool DecodeHexBlk(CBlock& block, const std::string hexblock)
+{
+ vector<unsigned char> blockData(ParseHex(hexblock));
+ CDataStream ssBlock(blockData, SER_NETWORK, PROTOCOL_VERSION);
+ try {
+ ssBlock >> block;
+ return true;
+ }
+ catch (const std::exception &) {
+ return false;
+ }
+}
+
+// NOTE: Assumes a conclusive result; if result is inconclusive, it must be handled by caller
+static Value BIP22ValidationResult(const CValidationState state)
@sipa
Bitcoin member
sipa added a note Nov 17, 2014

Pass by reference?

@luke-jr
Bitcoin member
luke-jr added a note Nov 17, 2014

Does it matter for const?

@sipa
Bitcoin member
sipa added a note Nov 17, 2014

Avoiding a copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa sipa commented on an outdated diff Nov 17, 2014
@@ -2555,6 +2567,32 @@ bool ProcessNewBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDis
return true;
}
+bool TestBlockValidity(CValidationState &state, const CBlock& block, CBlockIndex * const &pindexPrev, bool fCheckPOW, bool fCheckMerkleRoot)
+{
+ AssertLockHeld(cs_main);
+ assert(pindexPrev == chainActive.Tip());
+
+ CCoinsViewCache viewNew(pcoinsTip);
+ CBlockIndex indexDummy(block);
+ indexDummy.pprev = pindexPrev;
+ indexDummy.nHeight = pindexPrev->nHeight + 1;
+
+ // NOTE: CheckBlockHeader is called by CheckBlock
+ // NOTE: ContextualCheckBlockHeader only assigns ppindex, which we don't need/want
@sipa
Bitcoin member
sipa added a note Nov 17, 2014

Does it? It seems AcceptBlockHeader does...

EDIT: Ignore, I misread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@luke-jr
Bitcoin member
luke-jr commented Nov 17, 2014

Comments addressed and QA RPC tests added.

@TheBlueMatt TheBlueMatt and 1 other commented on an outdated diff Nov 18, 2014
qa/rpc-tests/getblocktemplate_proposals.py
+
+ def run_test(self):
+ node = self.nodes[0]
+ tmpl = node.getblocktemplate()
+ if 'coinbasetxn' not in tmpl:
+ rawcoinbase = encodeUNum(tmpl['height'])
+ rawcoinbase += b'\x01-'
+ hexcoinbase = b2x(rawcoinbase)
+ hexoutval = b2x(pack('<Q', tmpl['coinbasevalue']))
+ tmpl['coinbasetxn'] = {'data': '01000000' + '01' + '0000000000000000000000000000000000000000000000000000000000000000ffffffff' + ('%02x' % (len(rawcoinbase),)) + hexcoinbase + 'fffffffe' + '01' + hexoutval + '00' + '00000000'}
+ txlist = list(bytearray(a2b_hex(a['data'])) for a in (tmpl['coinbasetxn'],) + tuple(tmpl['transactions']))
+
+ # Test 0: Capability advertised
+ assert('proposal' in tmpl['capabilities'])
+
+ # NOTE: This test currently FAILS
@TheBlueMatt
TheBlueMatt added a note Nov 18, 2014

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

@luke-jr
Bitcoin member
luke-jr added a note Nov 18, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa sipa commented on an outdated diff Nov 18, 2014
src/main.cpp
@@ -2334,7 +2334,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, bool fCheckPOW, bo
return true;
}
-bool AcceptBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex** ppindex)
+bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& state, CBlockIndex** ppindex)
@sipa
Bitcoin member
sipa added a note Nov 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa
Bitcoin member
sipa commented Nov 18, 2014

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

@sipa
Bitcoin member
sipa commented Nov 18, 2014

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

@sipa sipa commented on an outdated diff Nov 18, 2014
src/rpcmining.cpp
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
+
+ uint256 hash = block.GetHash();
+ if (mapBlockIndex.find(hash) != mapBlockIndex.end()) {
@sipa
Bitcoin member
sipa added a note Nov 18, 2014

mapBlockIndex.count(hash) != 0 ?

@sipa
Bitcoin member
sipa added a note Nov 18, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj
Bitcoin member
laanwj commented Nov 19, 2014

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

@sipa sipa commented on an outdated diff Nov 19, 2014
src/rpcmining.cpp
throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "Block decode failed");
+
+ uint256 hash = block.GetHash();
+ BlockMap::iterator mi = mapBlockIndex.find(hash);
+ if (mi != mapBlockIndex.end()) {
+ CBlockIndex *pindex = mi->second;
+ if (pindex->nStatus & BLOCK_VALID_MASK == BLOCK_VALID_SCRIPTS)
@sipa
Bitcoin member
sipa added a note Nov 19, 2014

nit: pindex->IsValid(BLOCK_VALID_SCRIPTS)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@sipa
Bitcoin member
sipa commented Nov 19, 2014

Untested ACK.

@luke-jr
Bitcoin member
luke-jr commented Nov 20, 2014

(Made the change @sipa suggested.)

@laanwj laanwj added this to the 0.10.0 milestone Nov 20, 2014
@Diapolo Diapolo commented on the diff Nov 20, 2014
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);
@Diapolo
Diapolo added a note Nov 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
Bitcoin member
luke-jr added a note Nov 20, 2014

Better not to add redundant content.

@Diapolo
Diapolo added a note Nov 20, 2014

You are such a contra guy sometimes ^^.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@laanwj laanwj merged commit b867e40 into bitcoin:master Nov 24, 2014

1 check passed

Details continuous-integration/travis-ci The Travis CI build passed
@sdaftuar sdaftuar added a commit to sdaftuar/bitcoin that referenced this pull request May 14, 2015
@sdaftuar sdaftuar Merge 'upstream/master' and fix main.cpp merge conflict with #1816
Conflicts:
	src/main.cpp
76b297c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.