fix: reject non-hex block numbers in debug_getRawBlock, debug_getRawHeader, debug_getRawReceipts, and eth_getProof#10240
Conversation
9583b72 to
f464f4d
Compare
|
@macfarla since this fix is also a braking change, not sure IIRC about some kind of backward compatibility we want to keep, at least for some time. |
|
I will plan to review this week. Re the breaking change - we have flagged this as an upcoming breaking change so I think we just need to wait for 1 more release (I don't think we need to preserve the non-hex functionality). |
f464f4d to
690b112
Compare
macfarla
left a comment
There was a problem hiding this comment.
we don't want to target this change to specific RPC methods, we already have a BlockParameterOrBlockHash class that encapsulates this functionality but debug methods don't use it.
|
@macfarla all three debug methods are now aligned:
Could you take another look when you get a chance? Happy to adjust anything. |
|
@sridhar-panigrahi spotless check is failing. can you ensure you run spotless and tests locally. gradlew build now takes only about 20 min and will massively speed up the feedback cycle |
|
also will need a changelog entry |
|
@macfarla — the spotless and changelog issues from your last review are resolved. I also added a small follow-on change in the latest commits: |
yes please keep this PR focussed on the original fix |
|
@sridhar-panigrahi are you still working on this PR - if you remove the changes to Address I think it's pretty close to being ready |
|
@macfarla , I'll get this resolved by tomorrow I was occupied with my exams so wasn't able to look into this 😅 |
6cb67fe to
a0850ba
Compare
|
@macfarla rebased onto current main and dropped the |
macfarla
left a comment
There was a problem hiding this comment.
spotless is failing.
one last suggestion on hardcoded values in EthGeProofTest
Assuming all the tests pass (did you run them?) I'm about ready to approve
Replace hardcoded "0x1f4" / "0x1f5" with "0x" + Long.toHexString(blockNumber) and "0x" + Long.toHexString(blockNumber + 1) so the strings stay in sync with the blockNumber field if it ever changes. Signed-off-by: Sridhar Panigrahi <sridharpanigrahi2006@gmail.com>
|
@sridhar-panigrahi please fix spotless and confirm you have run the tests locally |
The hex-prefix check in BlockParameterOrBlockHash was rejecting inputs
like "-0x10" upfront with a generic IllegalArgumentException, which
methods mapped to INVALID_BLOCK_PARAMS ("Invalid block param (block
not found)"). The negative-number check already lives downstream in
AbstractBlockParameterOrBlockHashMethod and returns the more accurate
INVALID_BLOCK_NUMBER_PARAMS ("Invalid block number params") — accept
an optional leading minus so that path is reached.
Also update JsonRpcHttpServiceTest.ethGetStorageAtBlockNumber to pass
"0x0" instead of decimal "0" — the new contract is hex-only and this
test was the only remaining decimal usage in the api module.
Signed-off-by: Sridhar Panigrahi <sridharpanigrahi2006@gmail.com>
|
@macfarla ran the suite locally and it caught a real regression — the new hex check was rejecting |
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
yes running the tests caught a change in behavior! that is why we have them. In this case though, I think the negative hex carve out is confusing so I have pushed an update to those 4 tests (altering the expected error message) and removing the carve out. Running the checks now, and I'll have to get someone else to review. |
Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>
|
Thanks for picking that up, @macfarla — much appreciated. Agreed, one consistent rule reads cleaner than the carve-out, and the generic message is fine for a negative block number anyway. Looks good to me. 🙏 |
|
eth_getProof spec says BlockNumberOrTagOrHash but for all the debug_getRawXXX methods, it's BlockNumberOrTag so we need to make sure we match that. DebugGetRawReceipts should be changed to match, and changes to DebugGetRaw[Header,Block] should be reverted So actually, with the accompanying changes in other PRs, the only related hive test still failing is |
… spec debug_getRawBlock, debug_getRawHeader and debug_getRawReceipts now use BlockParameter (BlockNumberOrTag) instead of BlockParameterOrBlockHash, matching the execution-apis spec. Resolves the remaining debug_getRawReceipts/get-invalid-number hive failure. CHANGELOG breaking-changes list now explicitly names these three methods and eth_getProof (which keeps BlockParameterOrBlockHash per its spec). Signed-off-by: Sridhar Panigrahi <sridharpanigrahi2006@gmail.com>
|
Thanks @macfarla — good catch on the spec mismatch. Reverted DebugGetRawBlock and DebugGetRawHeader back to BlockParameter, and switched DebugGetRawReceipts the same way so all three line up with BlockNumberOrTag. eth_getProof stays on BlockParameterOrBlockHash since its spec allows hashes. Also tidied the CHANGELOG so the three debug_getRaw* methods and eth_getProof are listed by name. Local spotless + the touched tests are green — should clear that debug_getRawReceipts/get-invalid-number hive failure too. |
Summary
debug_getRawBlock,debug_getRawHeader,debug_getRawReceipts, andeth_getProofpreviously accepted decimal block numbers viaLong.decode()and silently succeeded. Hive'srpc-compatsuite sends"2"and expects-32602 INVALID_PARAMS.What this PR does:
BlockParameterandBlockParameterOrBlockHash, so every RPC using those parameter types gets the same validation. Named tags (earliest/latest/pending/finalized/safe) and block hashes still pass through.debug_getRawBlock,debug_getRawHeader, anddebug_getRawReceiptsareBlockNumberOrTag(notOrHash), so all three useAbstractBlockParameterMethod/BlockParameter. This is what closes the remainingdebug_getRawReceipts/get-invalid-numberhive failure.eth_getProofisBlockNumberOrTagOrHashper spec, so it stays onBlockParameterOrBlockHash— it just picks up the new hex validation for free.CHANGELOG breaking-changes entry now names
debug_getRawBlock,debug_getRawHeader,debug_getRawReceipts, andeth_getProofexplicitly, and the corresponding upcoming-breaking-changes note has been removed.refs #9636
Test plan
BlockParameterTest/BlockParameterOrBlockHashTestcover the new hex-prefix rejectionEthGetProofTestandDebugSetHeadTestupdated to use hex block numbers and passspotlessCheckcleanrpc-compat:debug_getRawBlock/debug_getRawHeader/debug_getRawReceipts/eth_getProofreject"2"with-32602 INVALID_PARAMS, and accept"0x2"/ named tags as before