wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members

achow101 commented at 10:37 pm on June 16, 2025: member

mapValue and vOrderForm are opaque data structures that contain transaction metadata. It is hard to determine what actual data each field contains, and they can ostensibly be misused where metadata is added in the future without developers realizing that such metadata exists.

It’s much clearer to have all of that metadata live in their own explicit member variables within CWalletTx. This PR implements that change.

Since the serialization format of CWalletTx depends on mapValue and vOrderForm, the serialization remains unchanged, so when serializing these new members, they need to be shoved/extracted from a temporary mapValue or vOrderForm.

This does end up breaking forwards compatibility as unknown fields in mapValue and vOrderForm are stripped out if the record is rewritten. However, I don’t expect that we would continue to use these fields for future metadata, so I think that risk is low.

wallet: Pass replaces_txid to CommitTransaction outside of mapValue

Instead of updating mapValue with the "replaces_txid" value and passing
the updated mapValue to CommitTransaction, pass the replaces_txid
directly to CommitTransaction which updates mapValue as necessary.

This is prepration for removing mapValue.

e79cc7d885

wallet: Pass comment and comment_to to CommitTransaction

Instead of passing these by setting them in mapValue, pass them directly
to CommitTransaction.

This is preparation for removing mapValue.

56e60a1f6b

wallet: Drop mapValue from CommitTransaction

The values previously passed in mapValue are now parameters to
CommitTransaction so there is no need for mapValue to be passed.

51e5472f01

DrahtBot commented at 10:37 pm on June 16, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32763.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32768 (wallet: Remove CWalletTx::fTimeReceivedIsTxTime by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Wallet on Jun 16, 2025

DrahtBot added the label CI failed on Jun 16, 2025

DrahtBot commented at 11:29 pm on June 16, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/44213782669 LLM reason (✨ experimental): The CI failure is caused by errors in clang-tidy checks due to calls to ’emplace_back’ inside loops in transaction.h, which are treated as errors.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

wallet: Make CWalletTx "from" and "message" member variables

Instead of storing "from" and "message" inside of mapValue, store these
explicitly as members of CWalletTx.

b5c830f7d4

wallet: Make CWalletTx "comment" and "to" member variables

Instead of storing "comment" and "to" inside of mapValue, store these
expliclty as members of CWalletTx.

3e8c97160c

wallet: Make CWalletTx "replaces_txid" and "replaced_by_txid" member variables

Instead of storing "replaces_txid" and "replaced_by_txid" as strings inside of
mapValue, store these expliclty as members of CWalletTx.

4499c32859

wallet: Drop mapValue from CWalletTx

It doesn't make sense to be storing relevant metadata variables inside
of a string map in CWalletTx. All of the fields have been pulled out
into separate members, so there is no need for mapValue to stick around.

c6d4dfcfa7

wallet: Drop vOrderForm from CommitTransaction

This parameter is only used to pass in the "Messages" from the GUI.
Instead of making it opaque by putting those into vOrderForm, use a
specific dedicated parameter for providing the messages.

82fb4fc5d5

wallet: Replace CWalletTx's vOrderForm with specific fields

vOrderForm contained 2 kinds of strings: BIP 21 messages, and BIP 70
Payment Requests. Instead of having both inside of a single vOrderForm
field that is opaque, split them into separate std::vector<std::string>
to contain this metadata.

398b632d5f

achow101 force-pushed on Jun 17, 2025

DrahtBot removed the label CI failed on Jun 17, 2025

ryanofsky commented at 10:49 am on June 17, 2025: contributor

Is forward compatibility a concern here? An potential advantage of serializing a list of fields instead of fixed set of fields is that it allows new fields to be added in the future and existing wallet software preserve the unknown fields instead of stripping them out.

Would be good to update PR the description to say what happens to unknown fields if a transaction is deserialized and reserialized. Are they stripped out? Preserved? Do they trigger an error?

achow101 commented at 6:04 pm on June 17, 2025: member

Is forward compatibility a concern here?

I don’t really think so. The last time these were modified was ages ago, I don’t foresee any need to continue to use mapValue or vOrderForm for new fields. Additionally, once a transaction is confirmed, it’s unlikely that the record would ever be modified again.

It would be straightforward to preserve mapValue and vOrderForm for any unknown values, but I think it’s unnecessary.

The primary motivation for this change is actually that I am working on a completely new storage mechanism for CWalletTx that utilizes SQLite as a SQL database, and I think the best way for that is to have each relevant field be its own column in a new “transactions” table.

Would be good to update PR the description to say what happens to unknown fields if a transaction is deserialized and reserialized.

Currently they would be stripped out. Updated the description.

wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members #32763

Code Coverage & Benchmarks

Reviews

Conflicts