docs: write early spec for tx client v2 #6224
Conversation
|
ninabarbakadze
requested review from
cmwaters,
evan-forbes,
rach-id and
rootulp
as code owners
rach-id
left a comment
rach-id
left a comment
There was a problem hiding this comment.
Thanks for the specs 🙏
I added some questions to understand more given I don't have any context regarding the mempool discussion
|
|
||
| Additionally I propose that we: | ||
|
|
||
| Drop mempool enforced TTLs and move TTL responsibility entirely to the client/user. The node no longer expires transactions; instead, callers control TTL via setting it as tx option. |
There was a problem hiding this comment.
What if they don't specify a TTL or provide a very low gas price and a very long ttl? we should have a fallback mechanism to avoid having transactions stuck in the mempool unnecessarily which would impact the other transactions.
Also, seems like if I make the mistake of sending a transaction with a very low gas price and a very long ttl, and the network has some activity and only including transaction that has a high gas price, my account will no longer be able to send tranactions given all of them will have a higher sequence and will not be accepted, and there is no way for me to remove my original transaction until the activity on the network slows down
There was a problem hiding this comment.
What if they don't specify a TTL or provide a very low gas price and a very long ttl?
Client would set it by default but I can imagine how this would fall through the cracks for submitting transactions without the client. We could make it required.
my account will no longer be able to send tranactions given all of them will have a higher sequence and will not be accepted, and there is no way for me to remove my original transaction until the activity on the network slows down
I think default mempool TTL is 30 blocks currently so we'd hit the same problem there, no?
There was a problem hiding this comment.
30 blocks currently so we'd hit the same problem there, no?
if I understand correctly, we would hit it for 30 blocks ~= 3min, but if there is no TTL, we could hit it indefinitely as long as there is congestion
There was a problem hiding this comment.
We need TTL on the protocol level, otherwise it will be easy DoS attack vector. Attacker would be able to spam the network with Txs without TTL or with very high TTL, with missing sequence numbers, etc.
evan-forbes
left a comment
evan-forbes
left a comment
There was a problem hiding this comment.
I don't think we should merge this yet, at least as a spec. It seems like a good explanation or discusison around some of the issues and some implementation details, however I don't think this should be used as a spec for a second rust based implementation.
for the spec @ninabarbakadze and I sync'd and concluded that roughly:
abstract: 3 sentences that describe the problem, high level guarantee, then expected outcome (of if the prototype is finished we can measure the outcome and confirm this implementation gets zero failures)
intro: continue from the high level problem introduced in the abstract and narrow down to specifics. In this case the high level guarantee we're actually providing is onchain ordering being the same as submission order.
actual spec: english as code (the client MUST do _____, the client SHOULD do _______). sequence diagrams are always helpful but ofc options
conclusion: a few sentences around to wrap up how the spec provides the outcome in the abstract
tzdybal
left a comment
tzdybal
left a comment
There was a problem hiding this comment.
Left some non-blocking comments.
| @@ -0,0 +1,207 @@ | |||
| # Tx Client v2 – Sequential Submission Queue | |||
|
|
|||
| ## Abstract | |||
There was a problem hiding this comment.
It's probably too obvious to us all, but I think it should be explicitly stated that this issue is related to submission from single account.
|
|
||
| Additionally I propose that we: | ||
|
|
||
| Drop mempool enforced TTLs and move TTL responsibility entirely to the client/user. The node no longer expires transactions; instead, callers control TTL via setting it as tx option. |
There was a problem hiding this comment.
We need TTL on the protocol level, otherwise it will be easy DoS attack vector. Attacker would be able to spam the network with Txs without TTL or with very high TTL, with missing sequence numbers, etc.
|
|
||
| ### Per account sequential queue | ||
|
|
||
| The basic idea is that every account gets its own **transaction lane**. |
There was a problem hiding this comment.
I would change a wording a bit - because the main idea of 'sequencial queue' is 'transaction lane'. And things are queued there.
Probably we can think of better names. My proposition:
- transaction queue
- transaction states: new/waiting, broadcasted.
Of there are no other possible, we can as well just expose boolean property Broadcasted.
|
|
||
| ## Abstract | ||
|
|
||
| The biggest pain point and bottleneck preventing 100% successful submissions is a race condition caused by eviction handling in tandem with our recovery mechanism in broadcast: |
There was a problem hiding this comment.
Consider defining/describing eviction and rejection.
| - **Still pending** → keep waiting | ||
| - **Committed** → great, return success | ||
| - **Committed with execution error** → return error | ||
| - **Evicted** → put it back in the queue to try again/directly resubmit since it's already signed | ||
| - **Rejected:** |
There was a problem hiding this comment.
Ok, so here are the possible Tx states 👍
IMHO it's good to just define a flat structure with all states (queued, broadcasted, commited, errored, evicted, rejected).
Overview
Fixes #6225