Optional late, block submission, difficulty test #22119

issue kanoi openend this issue on June 1, 2021
  1. kanoi commented at 12:56 pm on June 1, 2021: none

    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.

  2. kanoi added the label Feature on Jun 1, 2021
  3. maflcko added the label RPC/REST/ZMQ on Jun 1, 2021
  4. kanoi commented at 1:06 pm on June 1, 2021: none
    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.
  5. maflcko commented at 1:08 pm on June 1, 2021: member
    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.
  6. kanoi commented at 1:13 pm on June 1, 2021: none
    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
  7. maflcko removed the label Feature on Jun 1, 2021
  8. maflcko added the label Questions and Help on Jun 1, 2021
  9. sipa commented at 0:58 am on June 2, 2021: member
    @kanoi Note that @MarcoFalke is referring to the “block proposal” mode of getblocktemplate, not submitblock.
  10. kanoi commented at 2:31 am on June 2, 2021: none
    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)
  11. maflcko commented at 4:59 am on June 2, 2021: member

    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.

  12. maflcko closed this on Jun 4, 2021

  13. kanoi commented at 0:57 am on June 8, 2021: none

    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

  14. maflcko reopened this on Jun 8, 2021

  15. maflcko removed the label Questions and Help on Jun 8, 2021
  16. maflcko added the label Mining on Jun 8, 2021
  17. maflcko added the label Bug on Jun 8, 2021
  18. kanoi commented at 5:34 am on June 11, 2021: none

    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.

  19. kanoi commented at 11:42 am on September 2, 2022: none

    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)

  20. willcl-ark commented at 3:00 pm on October 14, 2024: member

    Would submitheader work for this use-case?

    Please comment here to have this issue re-opened if not.

  21. willcl-ark closed this on Oct 14, 2024


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me