Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Return more specific reject reason for submitblock #13983

Merged

Conversation

Projects
None yet
4 participants
@MarcoFalke
Copy link
Member

commented Aug 15, 2018

The second commit in #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.

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1808-rpcSubmitBlockInvalidReturn branch Aug 15, 2018

@MarcoFalke MarcoFalke changed the title rpc: Return more reject reject reason for submitblock rpc: Return more specific reject reason for submitblock Aug 15, 2018

@MarcoFalke MarcoFalke force-pushed the MarcoFalke:Mf1808-rpcSubmitBlockInvalidReturn branch to fa6ab8a Aug 15, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

No more conflicts as of last run.
@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 21, 2018

Concept ACK, don't know enough of the submit logic here to be confident that this is correct but the tests look convincing.

@laanwj laanwj added the Mining label Aug 21, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

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.

CC @TheBlueMatt @luke-jr

@ryanofsky
Copy link
Contributor

left a 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.

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Sep 13, 2018

Merge bitcoin#13983: rpc: Return more specific reject reason for subm…
…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 MarcoFalke merged commit fa6ab8a into bitcoin:master Sep 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarcoFalke MarcoFalke deleted the MarcoFalke:Mf1808-rpcSubmitBlockInvalidReturn branch Sep 13, 2018

@ryanofsky

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2018

@MarcoFalke, are you planning on adding release notes for this change or is it too minor to mention?

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.