Remove executed_in_epoch table.
#21477
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
mystenmark
force-pushed
the
mlogan-remove-executed-in-epoch
branch
from
34d17c6 to
8078644
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| } | ||
| batch.write()?; | ||
|
|
||
| if !matches!(tx_key, TransactionKey::Digest(_)) { |
There was a problem hiding this comment.
can this now be combined with above?
| } | ||
|
|
||
| // TODO: should this be debug_fatal? Its potentially very serious in that it could |
There was a problem hiding this comment.
on the question of "do we want this to halt the network?" my initial/gut feeling is yes, so that further damage will not continue to accrue that makes it even harder to recover, while we are trying to debug what happened and verify that it will not happen again.
There was a problem hiding this comment.
At the point at which this happens we are already most of the way through reconfig. The only damage will be that a transaction which achieved settlement finality has been dropped. Which is very bad for the sender, but it doesn't actually "damage" the chain in any other respect.
I'm comfortable leaving this as fatal because I believe the tests will catch such a bug before it makes it to prod. But if it did happen, i'm pretty sure the only thing we could do would be to push a fix which removes this fatal!. So that makes me wonder if there's any point.
| self.executed_in_epoch_cache.insert(tx_digest, ()); | ||
| } | ||
|
|
||
| // Called by CheckpointExecutor |
There was a problem hiding this comment.
nit - sort of? proximately it's called by authority per epoch store. I think this comment makes it a bit confusing, to expect to see a call to this fn in CheckpointExecutor but it's not there?
There was a problem hiding this comment.
Well, I guess I meant to indicate which component has a need for this method. I can rewrite the comment to be clearer.
|
Looking at this PR from a perspective "how can we test it better" - should we try to reduce the cache size to say 1-2 elements for the simtest runs to make sure we exercise path with cache eviction? It feels like with 50k cache capacity it might never even try this code path |
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
very good point |
mystenmark
force-pushed
the
mlogan-remove-executed-in-epoch
branch
from
270ae79 to
eb5eaee
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
The table is replaced with: - An in-memory "dirty set" which holds executed but un-checkpointed transaction digests. Transactions are removed from the dirty set by CheckpointExecutor. - An additional bounded cache intended to lessen the number of db reads by CheckpointBuilder - Last-resort reads go to the `executed_transactions_to_checkpoint` table. The only purpose of the table was to allow CheckpointBuilder to prune transaction dependencies from prior epochs, and the above approach suffices while removing a surprisingly expensive source of db writes.
mystenmark
force-pushed
the
mlogan-remove-executed-in-epoch
branch
from
eb5eaee to
7947247
Compare
mystenmark
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
The table is replaced with:
transaction digests. Transactions are removed from the dirty set by
CheckpointExecutor.
by CheckpointBuilder
executed_transactions_to_checkpointtable.The only purpose of the table was to allow CheckpointBuilder to prune
transaction dependencies from prior epochs, and the above approach
suffices while removing a surprisingly expensive source of db writes.