Optional late, block submission, difficulty test

[kanoi]

Software development for any bitcoin mining, solo or pooled, requires an excessively large hash rate to ensure block submission is correct.
Effectively to submit a full difficulty network block before there is some certainly of the validity of the block generation code developed.
Many developers do not test this properly and have lost blocks due to the developer's ignorance or negligence.

If instead the difficulty of the block submitted could optionally only be tested last, then testing block generation by submitting a low difficulty block, that would fail with {"result":"high-hash","error":null,"id":1} could directly imply that all other requirements and consensus tests have passed, only the low difficulty of the block has failed.
Implementing this would greatly simplify testing block generation code, since testing could be run on a bitcoin on the live network which would also be under the full validation and consensus rules of the live network.
A developer could simply stop their live bitcoin, restart it with the late difficulty validation flag and then run any testing required, then restart it without the flag.

This change would, of course, have to be optional, since saving one of the quickest and easiest tests of hashing the block header, moved much later in the validation, might open a DDoS exploit.
The bitcoin code could have two paths doing the difficulty validation and run one or the other depending upon the runtime flag.

Using testnet is not an exact test of the bitcoin block validation code due to varying code paths and varying consensus rules.

My normal process to test this is to hack the difficulty test in current bitcoin code and submit a lower difficulty block.
This however, means I must lock the development bitcoin behind a completely closed wall, since should it attempt to transmit the low difficulty block, it's IP would be appropriately ostracized by the live network.
The further result of this is the bitcoin having invalid blocks in it's database, which I have found it unable to roll back on a restart during re-validation with an unmodified bitcoin, and thus having to delete it.

Using a live online bitcoin, with a simple restart with a flag to enable full testing other than difficulty being last, should promote better development of mining generation software.

[maflcko]

How is this different from getblocktemplate proposal?

• https://bitcoincore.org/en/doc/0.21.0/rpc/mining/getblocktemplate/
• https://github.com/bitcoin/bips/blob/master/bip-0023.mediawiki#block-proposal

[kanoi]

It is no changes to any rules or validation, only one change to the order in which they are done, to optionally do the difficulty test last rather then where it is now done.

[maflcko]

They BIP says: "The block data MUST be validated and checked against the server's usual acceptance rules (excluding the check for a valid proof-of-work)", which is what your feature request is asking for, no? I am saying this is already implemented unless I am missing something.

[kanoi]

I'm under the impression that not all validity and consensus tests are done on a block submitted, that only fails the difficulty test, and is valid under all other rules.
I have purposely corrupted a transaction in the data submitted and still only get high-hash

[sipa]

@kanoi Note that @MarcoFalke is referring to the "block proposal" mode of getblocktemplate, not submitblock.

[kanoi]

Ah OK, I am referring to submitblock - but I'm not sure if he meant there is some method in getblocktemplate with the workid, to later use that workid to submit a low difficulty block with otherwise full testing.
Alas if there is, it's not obvious (and I can't see it explained in the BIPs)

[maflcko]

Alas if there is, it's not obvious (and I can't see it explained in the BIPs)

I agree this is a bit hidden, but I linked you the RPC documentation and the section in the BIP that explains this.

[kanoi]

Since this was changed to 'Help' ...
Alas, either I can't understand the code (i.e. my fault) or it doesn't work any more - I've tried this back to 0.17

The normal submit is of course is:
{"method":"submitblock","params":["blockdata"],"id":1}
With the expected result for a block without enough difficulty:
{"result":"high-hash","error":null,"id":1}

Looking at the rpc/mining.cpp code for 0.21.1 getblocktemplate when it sees the 'mode' of 'proposal' it says (skipping some parts):

const UniValue& dataval = find_value(oparam, "data");
DecodeHexBlk(block, dataval.get_str())
pindex = LookupBlockIndex(hash);
if (pindex) {
if (pindex->IsValid(BLOCK_VALID_SCRIPTS))
return "duplicate";
if (pindex->nStatus & BLOCK_FAILED_MASK)
return "duplicate-invalid";
return "duplicate-inconclusive";
}
TestBlockValidity(state, Params(), block, pindexPrev, false, true);
return BIP22ValidationResult(state);

So, if instead I do this with the same data:
{"method":"getblocktemplate","params":[{"mode":"proposal","data":"blockdata"}],"id":6}
The response is:
{"result":"bad-witness-nonce-size","error":null,"id":6}
Which seems to be incorrect.

So to verify this I remove the difficulty test from the code pow.cpp:
// Check proof of work matches claimed amount
// if (UintToArith256(hash) > bnTarget)
// return false;

Now the above submitblock replies:
{"result":null,"error":null,"id":1}
i.e. it's a valid block - ignoring difficulty
and the proposal returns:
{"result":"duplicate","error":null,"id":6}

So what I can make of all this is either the code is deprecated and not been updated, or there's some other data required for the proposal, that isn't needed until it gets to whatever causes "bad-witness-nonce-size"

So my suggestion, then, since a completely unexpected designed interface exists for submitting a proposal, that the 'submitblock' should be passed an optional 'proposal' and do the:
TestBlockValidity(state, Params(), block, pindexPrev, false, true);
return BIP22ValidationResult(state);

i.e. from the code point of view, how my original suggested change could be best implemented and used in a manner that makes sense using 'submitblock' not 'getblocktemplate'
Though the current TestBlockValidity() may or may not also need fixing, unrelated to my suggested change.

Edit: and in case it wasn't obvious, the line in validation.cpp that gives this error is:
if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) {
return state.Invalid(BlockValidationResult::BLOCK_MUTATED, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__));
}
But this doesn't fail when calling submitblock

[kanoi]

Though I've not yet attempted to test the change to determine if it is related to the TestBlockValidity() issue,
I've found there was a commit/merge to core on 1-Feb but it isn't in the 0.21.1 tag https://github.com/bitcoin/bitcoin/releases/tag/v0.21.1 that was released on 30-Apr

#20749
that modifies the call parameters and a later change to tidy up the calling and usage of the new parameter.

So I guess for the issue I've brought up about TestBlockValidity() possibly not working since at least 0.17, I'll shortly do some more testing with the current git to see if that's related or not.

[kanoi]

I've noticed that the TestBlockValidity() issue is still the same even with V23.0

Having read through the code to consider a simple implementation to place the POW check last when requested but leave it first if the latePOW isn't requested, I've found that the check code repeats many checks when it gets a block submission.

To be specific: src/rpc/mining.cpp submitblock() does some checks then calls src/validation.cpp ProcessNewBlock()

However ProcessNewBlock() repeats checks due to:
CheckBlock() followed by AcceptBlock()

These two overlap some checks and also do some different checks.
CheckBlock() calls:
CheckBlockHeader() <- this is the POW check
various validation checks
then finally flags the block as checked
AcceptBlock() calls:
AcceptBlockHeader()
CheckBlock() (again)
ContextualCheckBlock()
disk storage

Thus to optionally run the POW check last, it would have to disable it everywhere CheckBlockHeader() is called
and put it near the end of AcceptBlock()

I wonder if this convolution is due to the unnecessary TestBlockValidity() code that appears to be unsupported and poorly documented.

If anyone has the nous to work out how to implement this cleanly, that would be much appreciated.
RPC submitblock has a randomly documented optional 2nd parameter that could be used to add an optional "latepow" string to enable this. (or 'proposal' as suggested earlier, but 'latepow' seems to be more informative about it's action)