Nit: why the trailing comma?

Why is the default initialization changed to 0?

Why is the default initialization changed to 0?

From what I can tell in this PR, the convention mostly adhered to in this PR is to stop using -1 and ABANDON_HASH values at runtime, and restrict them only to the serialization code. This seems like a pretty good convention if it's followed consistently. I also think it'd be nice to explicitly say what values the block and index values should have for different states in a comment.

Yes see comment on top of TxConfirmation, should be improved ?

Can we move this backwards compatibility feature entirely to the serialization code, and then make nIndex a uint (for clarity)?

Can we move this backwards compatibility feature entirely to the serialization code, and then make nIndex a uint (for clarity)?

Personally -0 on this. Imo, unsigned types provide no safety and are terrible for everything except bitfields.

What do you mean here by moving this backwards compatibility feature entirely to the serialization code ? If changes are right this logic should only lived in serialization code now.

See below, it depends on if nIndex has any meaning.

Mention that nIndex is a block height?

With these changes, but even before, nIndex never referred directly to a block height. When set at -1, it's interpreted as a magic value meaning the hashBlock is one of the deepest conflicting tx. This PR intents to scope this logic only to serialization/deserialization.

If nIndex doesn't mean a block height, then why not remove the instance variable altogether? We can still (de)serialize in a backwards compatible way.

Well I think we need to keep nIndex at least it's used by RPCs in WalletTxToJSON ? Or getting it out of new TxConfirmation struct ?

I think we should figure out what it means in the RPC, consider deprecating that field, and then having an explicit backwards compatible workaround.

In the backwards compatiblity tests I wrote it seems blockindex is absent for uncofirmned or abandoned transactions. It was 1 for a confirmed transaction.

isConfirmed() does not seem to be tested as the others. Add a test? :-)

Removed for now, I've added it for further changes were ABANDON state was dual-set with other states, but drop if for now, would have been too big changes at once.

Should be "conflicted"? :-)

if hashBlock is null it means transaction is abandoned

Could you clarify this part of the comment. Doesn't 1 not 0 mean abandoned?

Currently, AbandonTransaction set nIndex as -1 and this value is kept at serialization/deserialization. I'm not sure if it make sense for older client, because an abandoned transaction can be also confirmed/unconfirmed/conflicted at same time. Nevertheless, I've tried to keep same behavior for now, so hashBlock set as ABANDON_HASH is used to avoid deserialization ambiguity with conflicted.

I've clarified this comment, the ABANDON_HASH one and also reset abandon txn hashBlock at deserialization. ABANDON_HASH should be scoped at serialization/deserialization.

Comment above about "if transaction was found in a block" no longer makes sense without the if condition.

Also, it seems like now when an abandoned, conflicted, or confirmed transaction is added to the mempool, and this is called, the transaction will now be marked unconfirmed instead of left in the previous state.

This seems logical, but I'm wondering if it is a change in behavior. It'd be helpful if the PR description or commit message would say explicitly whether any of this changes wallet behavior, and what the changes are if it does.

Clarified in commit message that is a change in behavior where at block disconnection, previous tx state is override by UNCONFIRMED one. I think right now, we don't track at all the fact that tx has been disconnected, and that's why we don't undo conflicts, or make it hard to solve them. The only direct consequence of these changes I can think of is a user having to call abandontransaction a 2nd time if block get disconnected and transaction is not back in mempool.

This change would make easier to track conflicts, where if conflicting transaction get UNCONFIRMED, iterate on conflicted outpoints and make them free.

Maybe, I can avoid update right now for UNCONFIRMED state, so a tx is born in this state but never go backward to it, and save implications for a later PR ?

I can live with changing it now, but we should drop the comment then.

In commit "Encapsulate tx state in a TxConfirmation struct" (8a7d26d)

On the surface it seems like improved behavior to not consider a transaction conflicted or abandoned when it's in the mempool. So just removing or updating the comment above seems good for that case. If this can mark a transaction not abandoned when it's not actually in the mempool that also seems ok but might be worth noting in a comment.

Ideally this commit might just be a pure refactor, and behavior changes could be left for other commits or PRs, but I think it's fine to have changes here if they're explicitly noted.

Can we move this backwards compatibility feature entirely to the serialization code, and then make nIndex a uint (for clarity)?

Personally -0 on this. Imo, unsigned types provide no safety and are terrible for everything except bitfields.

Just a suggestion, but I'd add = 0 here and = UNCONFIRMED on the line below to simplify constructor and deserialization code.

done

Why is the default initialization changed to 0?

From what I can tell in this PR, the convention mostly adhered to in this PR is to stop using -1 and ABANDON_HASH values at runtime, and restrict them only to the serialization code. This seems like a pretty good convention if it's followed consistently. I also think it'd be nice to explicitly say what values the block and index values should have for different states in a comment.

Can we set 0 here, or change MarkConflicted to set -1? I think it'd be good to be consistent about what conflicted transactions look like.

You're right that an inconsistency from my part. Corrected.

In commit "Remove SyncTransaction for conflicted txn in CWallet::BlockConnected" (0e3212a)

I don't think I understand this change. Commit description suggests why it is ok, but I don't understand what motivates it. Is it just cleanup? Is it maybe a minor bugfix, or needed for a future change?

Updated commit message. I think this redundant, as conflicts tagging logic is triggered by connection of conflicting transaction. AFAIK, it's not a bugfix but a cleanup. And as it was same issue than major commit, I bundled them together.

Can you clarify why we keep the first loop at all? Why not call TransactionRemovedFromMempool before SyncTransaction in the second loop?

Extended commit message, we keep the loop because set of conflicted txn isn't same as txn included in a block.

Can you clarify why we keep the first loop at all? Why not call TransactionRemovedFromMempool before SyncTransaction in the second loop?

Above this comment: For a CONFIRMED transaction, nIndex is the position inside the block, used in merkle proofs.

This had me confused before; I assumed it referred to a block height.

This serialization comment can be moved to the serialization code, because with this change nIndex is never -1; only serializedIndex is.

Moved to the deserialization code!

TxConfirmation tx_state is now a confusingly named variable.

Oh I've struggled hard on naming, TxConfirmation is struct with tx state + its values (block hash, position in block). If you have any better names, I'll take it :)

The variable name tx_state is the confusing part, because it's not a TxState

The variable name tx_state is the confusing part, because it's not a TxState

I think the current naming is ok, but if it I had to choose, I would probably do something like:

class CWalletTx
{
public:
    enum Status { UNCONFIRMED, CONFIRMED, CONFLICTED, ABANDONED };
    struct Confirmation {
        Status status;
        uint256 block_hash
        int pos_in_block;
    }
    ...
    Confirmation m_confirm;
    ...
};

Replaced by tx_conf on f9f4926

I can live with changing it now, but we should drop the comment then.

posInBlock could be a good rename candidate for nIndex, though probably not worth making the diff bigger.

In commit "Encapsulate tx state in a TxConfirmation struct" (8a7d26d)

On the surface it seems like improved behavior to not consider a transaction conflicted or abandoned when it's in the mempool. So just removing or updating the comment above seems good for that case. If this can mark a transaction not abandoned when it's not actually in the mempool that also seems ok but might be worth noting in a comment.

Ideally this commit might just be a pure refactor, and behavior changes could be left for other commits or PRs, but I think it's fine to have changes here if they're explicitly noted.

In commit "Encapsulate tx state in a TxConfirmation struct" (8a7d26d)

Should we maybe add

} else {
    assert(wtx.tx_state.nIndex == wtxIn.tx_state.nIndex);
    assert(wtx.tx_state.hashBlock = wtxIn.tx_state.hashBlock);
}

My concern is that the new behavior might be less robust than previous behavior if there is a bug in higher level code that marks a transaction as confirmed in two different blocks without marking the transaction as unconfirmed in between.

Done

(re: #16624 (comment))

I don't like these new asserts. I think they could be triggered under the following scenario:

1. a wallet tx is confirmed in a block
2. the wallet is shut down
3. the block chain re-orgs and the tx is included in a different block
4. the wallet file is loaded on a sync'ed node

All this code is trying to do is update an existing wallet tx's Confirmation status with the new Confirmation status. I think we can just change it to:

if (wtxIn.tx_state.state != wtx.tx_state.state || wtxIn.tx_state.hashBlock != wtx.tx_state.hashBlock || wtxIn.tx_state.nIndex != wtx.tx_state.nIndex) {

If these new asserts get triggered it would mean we have bugs in higher level code, but yes aborting maybe too strong given that this kind of bugs could be due to some likely API misuse in RPCs ? What's about logging them with a warning ?

I'm not in favor of proposed alternative as it may cause hidden inconsistencies if these aforementioned high level bugs are present

I don't feel strongly, but I think asserts are way to go because we should not allow bugs and uncertainty in wallet event processing code. It's hard for me to imagine a scenario where it would be a win to use defensive programming in a place like this.

@ryanofsky - do you agree that the scenario I describe above would trigger the assert?

Oh, I didn't realize #16624 (comment) was describing a specific scenario, not just a list of possible events. I could see that scenerio triggering these asserts, and making the change you suggested an improvement and bugfix. It is likely some change is needed here, and the suggested one seems ok.

More ideally, though, it'd be really nice to have a functional test that triggers these asserts, and instead of hiding the problem of an unconfirmed transaction being left in the CONFIRMED state after a reorg, there would be code on startup that marks reorged transactions UNCONFIRMED before rescanning (could basically be the same code from #15931 that looks up block heights on startup).

Nice catch though (assuming this a bug)!

I didn't realize #16624 (comment) was describing a specific scenario

sorry - changed my unordered list to an ordered list and updated wording to make this clearer.

there would be code on startup that marks reorged transactions UNCONFIRMED before rescanning

Yes - I think if the wallet rescans from block at height x, it should first change all transactions that are CONFIRMED at height >x to UNCOFIRMED.

I let asserts and move some bits from #15931 to transition status to UNCONFIRMED if blockHash not anymore in chain. Also added a wallet_reorgsrestore test with aforementioned scenario, without new code it triggers asserts

The variable name tx_state is the confusing part, because it's not a TxState

I think the current naming is ok, but if it I had to choose, I would probably do something like:

class CWalletTx
{
public:
    enum Status { UNCONFIRMED, CONFIRMED, CONFLICTED, ABANDONED };
    struct Confirmation {
        Status status;
        uint256 block_hash
        int pos_in_block;
    }
    ...
    Confirmation m_confirm;
    ...
};

In commit "Encapsulate tx state in a TxConfirmation struct" (8a7d26d)

It seems more safe to reset the whole struct instead of just one member. Maybe tx_state = TxConfirmation{};?

I think there may actually be a (theoretical) bug without this. If you unserialized an unconfirmed transaction into a CWalletTx that was not unconfirmed, deserialization code would not actually update the state to unconfirmed.

Well strictly speaking, given that Init set to unconfirmed, it would be more wrongly updating to confirmed. But if hash is set IMO we have database corruption or bug in higher tracking logic. Updated serialization logic with this and beneath to make it more robust.

In commit "Remove SyncTransaction for conflicted txn in CWallet::BlockConnected" (3e027c7)

Can we drop these tx_state.nIndex = 0; assignments or set them in all cases consistently? (There is a missing assignment in the unconfirmed case). I'd have a slight preference for just dropping these and the Init() call above reset everything to default, instead of repeating same default values multiple places.