Wallet handle txs with conflicting account nonces #1864
Conversation
azarovh
requested review from
erubboli,
ImplOfAnImpl and
OBorce
as code owners
azarovh
force-pushed
the
fix/wallet_remove_conflicting_txs
branch
2 times, most recently
from
3a2fd08 to
6c83ac1
Compare
|
|
||
| conflicting_txs.push(unconfirmed); | ||
| if let Some((confirmed_account, confirmed_account_nonce)) = confirmed_account_nonce |
There was a problem hiding this comment.
this can probably be an else if, as the transactions that use a token id will be a superset of the transactions that use the token id and have a specific nonce.
There was a problem hiding this comment.
I think that it'd make the code more confusing especially when modifying in the future. I replaced the vec with set to avoid duplicates.
azarovh
force-pushed
the
fix/wallet_remove_conflicting_txs
branch
from
8be3bb5 to
3327b22
Compare
|
Fixed the problem when abandoning a tx that is already marked as conflicted leads to invalid state of internal fields of OutputCache (like account nonces). |
| } | ||
| }, | ||
| }, | ||
| Entry::Vacant(_) => Err(WalletError::CannotFindTransactionWithId(tx_id)), |
There was a problem hiding this comment.
Is this also an invariant? If so, it deserves its own error.
But I also see that remove_tx removes the tx from txs unconditionally, possibly leaving it inside unconfirmed_descendants. If this situation is legitimate, we probably shouldn't return an error here.
There was a problem hiding this comment.
To me it looks like an invariant. Because removing a tx removes its descendants as well.
| TxState::Abandoned | ||
| | TxState::Confirmed(..) | ||
| | TxState::InMempool(..) | ||
| | TxState::Conflicted(..) => { | ||
| Err(WalletError::CannotChangeTransactionState( | ||
| *tx.state(), | ||
| TxState::Conflicted(block_id), | ||
| )) | ||
| } |
There was a problem hiding this comment.
This doesn't look nice. This whole function is called during block processing, so if this situation happens, the wallet will stop working, right?
Abandoned, Confirmed and Conflicted probably can't happen here and are invariant violations, am I right? If so, a separate error would be nice.
But what about InMempool? It sounds like it should be possible. If so, we must handle it somehow. Would it be wrong to change the state from InMempool to Conflicted?
There was a problem hiding this comment.
Looks like InMempool -> Conflicted is theoretically possible. Changed the logic to mark it as conflicted.
azarovh
force-pushed
the
fix/wallet_remove_conflicting_txs
branch
from
caa2c45 to
f87218f
Compare
| for conflicting_tx in conflicting_txs { | ||
| let txs_to_rollback = self.remove_from_unconfirmed_descendants(conflicting_tx); | ||
|
|
||
| // Mark conflicting tx and its descendants as Conflicting and update OutputCache data accordingly | ||
| for tx_id in txs_to_rollback.iter().rev().copied() { |
There was a problem hiding this comment.
There is a problem that currently I think cannot happen but can happen in the future when we add TXs from Mempool.
The problem is with the rollback_tx_data, as the TXs are iterated in random order by tx_id, in a situation like:
Having unconfirmed txs for a delegation or token:
A(nonce 1) -> B(2) -> C(3) -> D(4)
If for some reason the conflict happens in the middle of the chain e.g. nonce 2 (currently it should always happen at the start of the chain from confirmed blocks, unless I am mistaken)
So if it happens at 2, all 2, 3, and 4 will be inside conflicting_txs, but not sorted by tx_id. So the rollback can happen to finish at 3 or 4, which will be unable to find last_parent, the tx A as a still unconfirmed tx for this delegation or token.
There was a problem hiding this comment.
Thinking about it maybe you can have filter them out in n * log(d).
something like:
conflicting_descendents = BTreeSet::new();
for tx in conflicting_txs {
conflicting_descendents += find_descendents(tx);
}
conflicting_txs = conflicting_txs.filter(|tx| conflicting_descendetns.contains(tx))
This should also fix the double Conflict -> Conflict state
Unconfirmed txs in the wallet which uses outdated nonce are marked as conflicted now if confirmed tx is encountered on mainchain and the state of OutputCache is updated accordingly.
The behaviour is similar to abandoning tx.
Besides tests I had a wallet with conflicting orders which I couldn't open previously because of
InconsistentOrderDuplicateNoncewhich is now resolved.