-
Notifications
You must be signed in to change notification settings - Fork 277
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
fix: Smarter transaction expire logic #4802
Conversation
efb3850
to
d2781c2
Compare
d2781c2
to
1face71
Compare
1face71
to
51c158b
Compare
Signed-off-by: Sam H. Smith <sam.henning.smith@protonmail.com>
51c158b
to
12bc509
Compare
curr_time.saturating_sub(tx_creation_time) > time_limit | ||
curr_time.saturating_sub(tx_creation_time) + time_padding > time_limit |
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.
This change argues that more transactions should be expired in sumeragi, right?
Where does this issue originate?
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.
Yes. The original issue is that the leader can create a block with transactions that will expire before the consensus round is over. When iroha is under transaction load and producing many blocks this becomes very common. This change makes sure that block production remains smooth by throwing away transactions that are not guaranteed to live long enough.
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.
I thought it was at the time of block creation that determines whether transactions expire or not, and transactions only need to survive until the block creation.
Btw do we have some consensus mechanism on whether or not the leader should include a certain transaction in block, as noted here and here?
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.
I thought it was at the time of block creation that determines whether transactions expire or not, and transactions only need to survive until the block creation.
By looking at the code i think Sato is correct, we don't reject transaction from the block based on expired it or not.
is_expired
is Queue
method and we newer call in on block revalidation.
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.
Btw do we have some consensus mechanism on whether or not the leader should include a certain transaction in block, as noted here and here?
I far as i remember there were discussion that this mechanism is not really working because faulty peer can ignore sending receipt or smt like that.
EDIT: Relevant disscussion was here #2636
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.
is_expired
is Queue method and we newer call in on block revalidation.
which seems like a bug because malicious leader can put expired transactions into the new block. This should be checked during consensus by other peeers. But only during consensus and not during block replay
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.
this change might cause some problems because time is not synchronized across nodes
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.
Regardless of this PR, correct implementation of TTL might not be possible without time consensus:
the leader imposes a time window (with Queue.tx_time_to_live
and Queue.future_threshold
) on transactions from a client, but since there is no time consensus, even if the leader drops all transactions with its system time far away from the client's one, no one can sue the leader with certainty
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.
But it may also be true that there is no incentive for leaders to cheat in such a way. For example, they can't just ignore certain clients this way. Ignoring all transactions, they would simply result in being replaced immediately
EDIT:
Some clients might collude with the leader to shift the time and allow only their transactions to pass. But that's only for that round, and I'm not sure how much harm it can do
Description
Linked issue
Closes #{issue_number}
Benefits
Checklist
CONTRIBUTING.md