-
Notifications
You must be signed in to change notification settings - Fork 25
add transaction time validity #571
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
Conversation
1f7a6e9 to
bb24cdc
Compare
bb24cdc to
79e85ea
Compare
eugene-babichenko
left a comment
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.
While the change itself looks good, I am wondering how would it affect the system in certain edge cases? E.g. a transaction sitting in the mempool for too long because the network is not able to process the influx of transactions any faster. We did not encounter such situation before with jormungandr, but IMO this is something that must be taken into consideration.
mzabaluev
left a comment
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.
Good work!
chain-impl-mockchain/doc/format.abnf
Outdated
|
|
||
| IOW = SIZE-ELEMENT-8BIT ; number of inputs | ||
| SIZE-ELEMENT-8BIT ; number of outputs | ||
| BLOCK-DATE ; start validity of this IOW |
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.
The start date is not present in the data structure yet.
| .make_input_with_value(self.builder.fee(&self.cert)); | ||
| self.builder | ||
| .fragment(&self.cert, keys, &[input], &[], false, &self.funder) | ||
| self.builder.fragment( |
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 must say this builder API is not very builder-y, and has become even less so.
a6a94a5 to
6129a0f
Compare
Each transaction comes with a start and end block date, which are inclusive bound of validity: start <= current_date <= end and where start < end. if transaction contains the start=0.0, end=0.0 blockdates, we expects the same behavior as before: no expiration of validity Originally the end was relative number of slots from the start, but this creates lots of difficulty to calculate the end precisely, due to possible era changes, so instead this is now a blockdate that doesn't have to match the number of slots per epoch.
expect the start validity to be defined implicitely by the node's mempool's blockdate
introduce a new SetValidity state in the transaction builder lots of minor adjustment in the test suite to cope with explicit date verification now in the ledger.
6129a0f to
059fb9a
Compare
Remember that a transaction broadcasted to the p2p network is not guaranteed to be added by a leader node. It dependent on multiple variables: how well the transaction is broadcasted to the network, how full the mempool currently are (in Jörmungandr, we use a LRU cache. So a transaction seating for too long is going to be pruned anyway). Adding this time validity is mostly to allow the user to set a deterministic mechanism to detect a transaction is considered invalid. This will allow many users to have a less stressful experience using Jörmungandr. Imagine you send your transaction and you have to wait days and it is still not in the blockchain? Price fluctuates a lot at the scale of days in the cryptocurrency industry. And this transaction may be important: paying the rent for example. There is no way to know if the transaction is going to be included or not in the blockchain and when. Between that instant you broadcast your transaction and that it appears in the blockchain: there is a laps of time where things are up in the air. |
Ah yes, but we dismantled that because it could be a way to censor or otherwise disadvantage other participants by flushing out their transactions. |
Exactly. Also, it provides a more deterministic way to prune the mempool. |
I appreciate you want to be fair with the users. Good for you. Though I don't see how this is fair/unfair. Every nodes can set the mempool to allow as much backlog as possible with appropriate cache size. |
so while the user side and mempool management, are both very important and wanted side effect of this, the main reason for the end validity is for a further improvement related to account nonce, so that we can introduce non-monomorphically-strictly-increasing nonce. |
All names now have "expiry", set_validity had too generic meaning.
A dishonest participant could still guess sizes of configured mempools, or just try to flush out unwanted fragments with statistically significant results. |
|
thanks @vincenthz , helpful to have that comment. I now remember you talking about that back in the day. @mzabaluev this kind of strategy only works for a time and is very expensive to put in place. You should trust the ledger's protocol more to deal with the transaction flooding. The node is not affected at all by that kind of behaviour and having a LRU prevented the mempool from growing into unmanageable proportions. |
|
Wouldn't that (LRU eviction) violate the liveness property as described in the Ouroboros-BFT paper?
|
the main purpose is to give a range of liveness for a given transaction,
with the possibility of garbage collecting old expired transaction from mempool.
Each transaction comes with a start and end block date, whichare inclusive bound of validity: start <= current_date <= end
and where start < end.
if transaction contains the start=0.0, end=0.0 blockdates, weexpects the same behavior as before: no expiration of validity
Each transactions contains now a block date until when the transaction is valid.
after this date, the transaction cannot be applied to the ledger anymore.
Originally the end was relative number of slots from the start,
but this creates lots of difficulty to calculate the end
precisely, due to possible era changes, so instead this is now
a blockdate that doesn't have to match the number of slots per epoch.