[Fix] Ensure aborted transactions are handled properly on sync#4138
[Fix] Ensure aborted transactions are handled properly on sync#4138
Conversation
kaimast
force-pushed
the
fix/load-aborted-transmissions
branch
from
362e9e9 to
b06e695
Compare
| { | ||
| // The solution was already contained in the ledger. | ||
| missing_transmissions.insert(*transmission_id, solution.into()); | ||
| } else { |
There was a problem hiding this comment.
I'm missing the ledger.contains_transmission call for aborted transmissions
| // was aborted before querying the ledger. | ||
| if let Some(transaction) = unconfirmed_transactions.get(transaction_id) { | ||
| missing_transmissions.insert(*transmission_id, transaction.clone().into()); | ||
| } else if aborted_transactions.contains(transaction_id) { |
There was a problem hiding this comment.
Same here about ledger.contains
kaimast
force-pushed
the
fix/load-aborted-transmissions
branch
from
b06e695 to
871ed93
Compare
|
I updated the PR and addresses @vicsn's comments. It now includes the workaround to log a warning when a transaction is not found, instead of aborting. |
| #path = "../snarkVM" | ||
| git = "https://github.com/ProvableHQ/snarkVM.git" | ||
| rev = "8df377d" | ||
| rev = "bb5c8ae6" |
There was a problem hiding this comment.
This revision needs to be updated, as we already have snarkOS changes depending on very recent snarkVM revisions. Actually, it seems that the current value is fine, this line just needs to be reverted.
There was a problem hiding this comment.
I did a rebase and removed this change.
ljedrz
left a comment
ljedrz
left a comment
There was a problem hiding this comment.
LGTM, but needs rebasing.
kaimast
force-pushed
the
fix/load-aborted-transmissions
branch
from
e83b624 to
25114de
Compare
|
I made some fixes to this logic, but I don't want to rebase #4039 again, so I will close this PR. |
3 participants
This PR introduces two small, but important, improvements to block sync.
c1b4d7a updates the snarkVM rev to leverage the new
try_getfunctionality for transmissions (ProvableHQ/snarkVM#3117). This change also makes errors propagate to the calling function, instead of simply logging a warning.362e9e9 ensures that the sync logic checks whether a transaction was aborted before querying the ledger. In the current implementation the ledger returns an error for aborted transmissions, as there exists a reference to it in the ledger but the transmission itself is not stored in the ledger.
This should probably be fixed on the snarkVM-side, but checking the aborted set first is also cheap and avoids expensive rocksdb lookups in some cases.
I made this a separate PR as the changes are self-standing and provide a useful improvement.