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

Padding of the R and S components in case they are smaller than 31 bytes for SECP256r1 #52

Merged

Conversation

freemanzMrojo
Copy link
Contributor

@freemanzMrojo freemanzMrojo commented Jan 25, 2022

Note from @NicolasMassart (Consensys Customer Support):
This PR if from one of the Consensys customers directly. I removed the internal reference to our ticketing system as it's not useful to HL community as they don't have access. The PR is well referenced in the ticket on our side.

Description of the issue

Sometimes the signature components (in that case it was R) are 30-bytes long and both the keys and the signatures are still valid.

The values added in the unit test are taken from one of the actual errors shown in the error logs:

2022-01-19 17:39:09.672+00:00 | EthScheduler-Workers-1 | INFO  | PersistBlockTask | Imported #591,672 / 22 tx / 0 om / 784,208 (0.0%) gas / (0x14eb9590af60f2e86d4a617bb34e8de4255b35819cfbb8a1a87dc0cbbc021b21) in 0.065s. Peers: 4
2022-01-19 17:39:10.032+00:00 | vert.x-worker-thread-3 | ERROR | JsonRpcHttpService | Error processing JSON-RPC requestBody
java.lang.IllegalStateException: Cannot recover public key from signature for MessageCall{type=FRONTIER, nonce=354537, gasPrice=0x0, gasLimit=900000, to=0x4bdf5ccc513a0f4c9e70f8e22dc572c6a3ce93f3, value=0x0000000000000000000000000000000000000000000000000000000000000000, sig=Signature{r=607232317131644998607993399928086035368869502933999419429470745918733484, s=909326537358980219114547956988636184748037502936154044628658501523731230682, recId=1}, payload=0x2b96ec34000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000ce080112a3010a14d4e799019fb8850f5532baafb57302ae7ad4daad1220c57f3ba81cb7fd341e18dcbbf29ee2e1de8fa0253099304f7fd92088b895f5d51a473045022100ef44c089e8e099cabea07b002985876c543194b0bc00415fef8cee5b8f6e8e6b02205580d481f79fb1f6ee2b541738b79481d89c159c3c39cd9b438d3e52c29065122220088116856e9d1d554d9f0d51de6405b25d1f5eabca81f7a3781145830f975a4f2a2431643265633838612d626431372d343565342d383535622d653962656536363565666331000000000000000000000000000000000000}
	at org.hyperledger.besu.ethereum.core.Transaction.lambda$getSender$3(Transaction.java:517)
	at java.base/java.util.Optional.orElseThrow(Optional.java:408)
	at org.hyperledger.besu.ethereum.core.Transaction.getSender(Transaction.java:515)
	at org.hyperledger.besu.ethereum.mainnet.MainnetTransactionValidator.validateTransactionSignature(MainnetTransactionValidator.java:257)
	at org.hyperledger.besu.ethereum.mainnet.MainnetTransactionValidator.validate(MainnetTransactionValidator.java:111)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionPool.validateTransaction(TransactionPool.java:240)
	at org.hyperledger.besu.ethereum.eth.transactions.TransactionPool.addLocalTransaction(TransactionPool.java:135)
	at org.hyperledger.besu.ethereum.api.jsonrpc.internal.methods.EthSendRawTransaction.response(EthSendRawTransaction.java:79)
	at org.hyperledger.besu.ethereum.api.jsonrpc.JsonRpcHttpService.process(JsonRpcHttpService.java:748)
	at org.hyperledger.besu.ethereum.api.jsonrpc.JsonRpcHttpService.lambda$handleJsonSingleRequest$14(JsonRpcHttpService.java:608)
	at io.vertx.core.impl.ContextImpl.lambda$null$0(ContextImpl.java:159)
	at io.vertx.core.impl.AbstractContext.dispatch(AbstractContext.java:100)
	at io.vertx.core.impl.ContextImpl.lambda$executeBlocking$1(ContextImpl.java:157)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:829)
2022-01-19 17:39:11.668+00:00 | EthScheduler-Workers-1 | INFO  | PersistBlockTask | Imported #591,673 / 11 tx / 0 om / 392,072 (0.0%) gas / (0xda0059d5397a909d9f7ffd5b178e323da9f3623a36eb0d60843209ff8d8ab894) in 0.036s. Peers: 4

There is another test created in this fork on an upper level for Besu checking precisely the same as this test.

Fix

Add proper padding to signatures

Signed-off by: miguelangel.rojofernandez@mastercard.com

Signed-off-by: Miguel Rojo <miguelangel.rojofernandez@mastercard.com>
Signed-off-by: Miguel Rojo <miguelangel.rojofernandez@mastercard.com>
Signed-off-by: Miguel Rojo <miguelangel.rojofernandez@mastercard.com>
@freemanzMrojo freemanzMrojo force-pushed the signature-components-of-30-bytes branch from 67bf304 to 18948a5 Compare January 25, 2022 18:08
@NicolasMassart NicolasMassart self-assigned this Jan 26, 2022
Copy link

@daniellehrner daniellehrner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small change in one of the comments. The rest seems good to me.

@NicolasMassart NicolasMassart added the bug Something isn't working label Jan 26, 2022
Signed-off-by: Miguel Rojo <miguelangel.rojofernandez@mastercard.com>
Copy link

@daniellehrner daniellehrner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@daniellehrner daniellehrner merged commit 0353a6f into hyperledger:main Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants