fix: Smarter transaction expire logic #4802
Conversation
SamHSmith
requested review from
s8sato,
mversic,
outoftardis,
Erigara,
0x009922,
DCNick3 and
dima74
as code owners
SamHSmith
force-pushed
the
smarter_tx_expire
branch
from
efb3850
to
d2781c2
Compare
SamHSmith
changed the title
SamHSmith
force-pushed
the
smarter_tx_expire
branch
from
d2781c2
to
1face71
Compare
SamHSmith
changed the title
SamHSmith
force-pushed
the
smarter_tx_expire
branch
from
1face71
to
51c158b
Compare
Signed-off-by: Sam H. Smith <sam.henning.smith@protonmail.com>
SamHSmith
force-pushed
the
smarter_tx_expire
branch
from
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.
This change argues that more transactions should be expired in sumeragi, right?
Where does this issue originate?
There was a problem hiding this comment.
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.
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.
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.
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.
is_expiredis 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.
this change might cause some problems because time is not synchronized across nodes
There was a problem hiding this comment.
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.
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