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
| | AccountCommand::FillOrder(_, _, _) => None, | ||
| AccountCommand::FreezeToken(frozen_token_id, _) => Some(frozen_token_id), | ||
| }, | ||
| let conflict = confirmed_tx.inputs().iter().find_map(|input| { |
There was a problem hiding this comment.
I guess this should be a collection. And updating it should probably take into account the fact that later inputs may increment a nonce that has already been incremented on a previous iteration.
P.S. and it's better to have a test for this.
| if let WalletTx::Tx(tx) = unconfirmed_tx { | ||
| if let Some(frozen_token_id) = conflict.frozen_token_id { | ||
| if self.uses_token(unconfirmed_tx, &frozen_token_id) { | ||
| conflicting_txs.insert(tx.get_transaction().get_id()); |
There was a problem hiding this comment.
Btw the later if becomes pointless if we get here. I'd suggest rewring the whole thing as
let mut is_conflicting = false;
if let Some(frozen_token_id) = conflict.frozen_token_id {
if ...
is_conflicting = true;
}
if !is_conflicting {
if ...
is_conflicting = true;
}
if is_conflicting {
conflicting_txs.insert(...);
}
| // because unconfirmed_descendants contains a tx as child and as parent. | ||
| // So it's not an error only if done during this function call. | ||
| ensure!( | ||
| conflicting_txs_with_descendants.contains(&tx_id), |
There was a problem hiding this comment.
-
Let's make
conflicting_txs_with_descendantsa set? -
I'm now wondering whether returning an error makes sense here. I mean, if we get here, the cache is in a half-updated state. Probably crashing might be a safer option here. Though I'm not sure. (And we do return errors on other invariant violations in this function).
| orders: &mut BTreeMap<OrderId, OrderData>, | ||
| ) { | ||
| let tx_id = tx.get_transaction().get_id(); | ||
| for input in tx.get_transaction().inputs() { |
There was a problem hiding this comment.
I guess we should go in the reverse order here, to handle the situation where the same account is modified twice in the same tx.
A test for this would be nice to have as well.
| TxInput::Utxo(outpoint) => { | ||
| self.consumed.insert(outpoint.clone(), tx.state()); | ||
| if is_unconfirmed { | ||
| self.unconfirmed_descendants | ||
| .get_mut(&outpoint.source_id()) | ||
| .as_mut() | ||
| .map(|descendants| descendants.insert(tx_id.clone())); | ||
| } else { | ||
| self.unconfirmed_descendants.remove(tx_id); | ||
| if let Some(descendants) = | ||
| self.unconfirmed_descendants.get_mut(&outpoint.source_id()) | ||
| { | ||
| ensure!( | ||
| is_unconfirmed, | ||
| WalletError::ConfirmedTxAmongUnconfirmedDescendants(tx_id.clone()) | ||
| ); | ||
| descendants.insert(tx_id.clone()); | ||
| } | ||
| } | ||
| TxInput::Account(outpoint) => match outpoint.account() { |
There was a problem hiding this comment.
To me, both the old and the new version seem wrong.
The old one seems wrong because of else { self.unconfirmed_descendants.remove(tx_id); } - though tx itself is confirmed, it can still have unconfirmed descendants and the remove drops them all for no reason.
The new version adds tx as an unconmfirmed descendant to its parent only if the parent already has some unconfirmed descendants. Also, the ensure! seems to prohibit update_inputs from being called on a confirmed tx, if one of its parents has unconfirmed descendants.
I guess the correct way of going about this is to first write 3 tests: the 1st one should fail for the old version due to extra unconfirmed descendants being dropped, the 2nd should fail for the new version due to tx not being added as an unconfirmed descendant, the 3rd one should fail for the new version as well due to update_inputs failing for a confirmed tx.
| // Removes a tx from unconfirmed descendant. | ||
| // Returns provided tx and all the descendants. | ||
| fn remove_from_unconfirmed_descendants( |
There was a problem hiding this comment.
The other thing that its caller depends on is the fact that children always come after the parents in the result. It makes sense to mention this as well.
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.