-
Notifications
You must be signed in to change notification settings - Fork 0
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
improve tx forwarding race condition #74
base: main
Are you sure you want to change the base?
Conversation
func (c *Cache) CurrentHeight() int64 { | ||
return atomic.LoadInt64(&c.currentHeight) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my pr I'm actually changing currentHeight to be an atomic.Int64 for more ergonomic use, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no preference, I think I like it typed how you did so i'll deal with the conflicts!
// exclude deadlines that are more than 120 blocks behind the current block | ||
deadline = currentBlock + 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems misleading, we're not actually excluding the deadline in this condition. Should the comment read "exclude deadlines that are more than 120 blocks behind the current block unless transaction is recent (came from syncing node)"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! way better
adds a deadline, submitted_at, and forwarded_from to the forward tx request
uses these to implement some gating at the reception level so old txs get ignored and syncing nodes can still send them
if a tx has a deadline too far the receiving node sets it back to 10 blocks away
if it's from a long time ago blockwise but the timestamp is recent then the node accepts it, likely from syncing node. this should be hardened later