rpc: Return more specific reject reason for submitblock #13983
Conversation
fa3d2b3 to
facb100
Compare
facb100 to
fa6ab8a
Compare
| No more conflicts as of last run. |
|
Concept ACK, don't know enough of the submit logic here to be confident that this is correct but the tests look convincing. |
|
Yeah, submitblock had zero test coverage previously, which is why this was missed during review I guess. Personally I don't care about the specificness of the return code for invalid blocks (since in normal operation you'd never submit invalid blocks), but we might want to backport this if some miner cares. |
There was a problem hiding this comment.
utACK fa6ab8a. I'm not very familiar with this logic, but the code change is simple and matches the description, and the new test coverage is good.
Two possible suggestions:
- Maybe add the new test cases in one commit, and then make the code change in a second commit so the test diff can reflect the changed behavior.
- Maybe add release note mentioning the change.
…itblock fa6ab8a rpc: Return more specific reject reason for submitblock (MarcoFalke) Pull request description: The second commit in bitcoin#13439 made the `TODO` in the first commit impossible to solve. The meaning of `fNewBlock` changed from "This is the first time we process this block" to "We are about to write the new *valid* block". So whenever `fNewBlock` is true, the block was valid. And whenever the `fNewBlock` is false, the block is either valid or invalid. If it was valid and not new, we know it is a `"duplicate"`. In all other cases, the `BIP22ValidationResult()` will return the reason why it is invalid. Tree-SHA512: 4b6edf7a912339c3acb0fccfabbdd6d812a0321fb1639c244c2714e58dc119aa2b8c6bf8f7d61ea609a1b861bbc23f920370fcf989c48452721e259a8ce93d24
|
@MarcoFalke, are you planning on adding release notes for this change or is it too minor to mention? |
|
@ryanofsky I believe the changes here only affect the return value of invalid blocks. I'd be surprised to see someone submitting invalid blocks in production. Also, I believe this restores the behavior that existed before 0.17.0. If someone feels strongly they could backport that to 0.17.0, but at this point it might be too late to get in. |
The second commit in #13439 made the
TODOin the first commit impossible to solve.The meaning of
fNewBlockchanged from "This is the first time we process this block" to "We are about to write the new valid block".So whenever
fNewBlockis true, the block was valid. And whenever thefNewBlockis false, the block is either valid or invalid. If it was valid and not new, we know it is a"duplicate". In all other cases, theBIP22ValidationResult()will return the reason why it is invalid.