internal/ethapi: add eth_SendRawTransactionSync #32830
Conversation
aodhgan
requested review from
fjl,
lightclient,
rjl493456442 and
s1na
as code owners
aodhgan
changed the title
|
We can do a slight refactor with this PR merged: #32697 (comment) |
This PR was merged ^ |
aodhgan
force-pushed
the
ag/feat/add-ethsendRawTransactionSync
branch
from
0daa396 to
3a3c46a
Compare
The given examples (missing nonce or insufficient funds) are already covered by their own more granular codes (https://github.com/aodhgan/go-ethereum/blob/ag/feat/add-ethsendRawTransactionSync/internal/ethapi/errors.go#L102). There is an option to coalesce these into the new error code 5 but dont see much value? (Also considering the EIP says SHOULD (vs MUST)). The "node not ready to accept new transactions" seems to require a deeper set of changes? wdyt @s1na? |
Got it, that makes sense. Since the EIP says SHOULD and not MUST, it’s optional. I just wanted to check if we plan to follow that or keep using the current detailed error codes. |
On the nod ready-ness we could query and see if it is in sync or not, but that's not something we do for the other sendTransaction variants. I'm not sure why it was introduced to this method. On the other point I think I agree that coalescing means losing info, and given the wording, I'd go with the current approach. |
ethclient/ethclient.go
Outdated
| timeout time.Duration, | ||
| ) (*types.Receipt, error) { | ||
| var buf bytes.Buffer | ||
| if err := tx.EncodeRLP(&buf); err != nil { |
There was a problem hiding this comment.
What about naming it SendTransactionSync? and the other one SendRawTransactionSync
There was a problem hiding this comment.
Also I think timeout should be optional
|
|
||
| // SendRawTransactionSync will add the signed transaction to the transaction pool | ||
| // and wait until the transaction has been included in a block and return the receipt, or the timeout. | ||
| func (api *TransactionAPI) SendRawTransactionSync(ctx context.Context, input hexutil.Bytes, timeoutMs *hexutil.Uint64) (map[string]interface{}, error) { |
There was a problem hiding this comment.
Ah it's annoying that the spec specifies the timeout in ms. In other places we use string like 10m or 20s. But oh well.
| select { | ||
| case <-receiptCtx.Done(): | ||
| // Upstream cancellation -> bubble it; otherwise emit the timeout error | ||
| if err := ctx.Err(); err != nil { |
There was a problem hiding this comment.
If deadline has passed it will still be returned here as an error. You can check the error against DeadlineExceeded to catch that case and return the proper error code.
internal/ethapi/api.go
Outdated
|
|
||
| case err, ok := <-subErrCh: | ||
| if !ok || err == nil { | ||
| // subscription closed; disable this case |
There was a problem hiding this comment.
I don't understand why you don't return at this point? am I missing something?
There was a problem hiding this comment.
think that was a stale case from a another implementation i tried. changed in
409eea4
6 participants
New RPC method eth_sendRawTransactionSync(rawTx, timeoutMs?) that submits a signed tx and blocks until a receipt is available or a timeout elapses.
Uses a ChainEvent subscription to grep receipts and filter for the receipt of interest.
Two CLI flags to tune server-side limits:
--rpc.txsync.defaulttimeout (default wait window)
--rpc.txsync.maxtimeout (upper bound; requests are clamped)
Success/timeout unit tests added.
closes #32094