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.

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.

Type	Reviewers
ACK	w0xlt
Concept NACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32966 (Silent Payments: Receiving by Eunovo)
#28201 (Silent Payments: sending by josibake)
#27865 (wallet: Track no-longer-spendable TXOs separately 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.

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.

ryanofsky commented at 5:32 pm on June 18, 2025: contributor

I guess I’m wondering a little more about forward compatibility. In general, it seems like it could be a useful thing for future wallet software to be able to add metadata to transactions that older software will preserve and ignore. If the deserialization code is changed to now discard unrecognized mapvalue fields instead of preserving them, are there different places new transaction metadata could be stored safely in the future? Or is mapvalue’s extensibiilty just less useful than it might seem, and if we added new transaction fields maybe we’d always want to forbid opening the new wallets with old software anyway? Alternately I’d wonder if CWalletTx::Unserialize should raise an error if an unrecognized field is present, so it does not appear to deserialize successfully when it might discard information. Preserving unknown fields does seems like the most conservative option here, but you know a lot more about this, so feel free to go with whatever approach make the most sense.

achow101 commented at 5:45 pm on June 18, 2025: member

Or is mapvalue’s extensibiilty just less useful than it might seem

This, generally. What we usually do for upgrading metadata is to put the new metadata at the end of the record. Deserialization always allows for there to be extra data, so it’s always backwards compatible to stick things onto the end. The only compatibility issue is if the record is rewritten during a downgrade followed by a re-upgrade. But we also have the wallet flags that we use for downgrade protection if that is necessary.

I don’t think a new field has been added to mapValue or vOrderForm in about a decade too, and I don’t think anyone is planning to add any new fields any time soon.

DrahtBot added the label Needs rebase on Jun 25, 2025

achow101 force-pushed on Jun 25, 2025

DrahtBot removed the label Needs rebase on Jun 25, 2025

luke-jr commented at 6:27 pm on June 26, 2025: member

Concept NACK, agree with @ryanofsky’s concerns. I don’t think there’s any benefit to refactoring this either?

achow101 commented at 8:50 pm on June 26, 2025: member

I don’t think there’s any benefit to refactoring this either?

There is a concrete benefit to developers for knowing exactly what metadata fields are present in CWalletTx. Both mapValue and vOrderForm are opaque - it’s not obvious what data they store just by looking at CWalletTx’s definition. Instead, you have to grep around the codebase for mapValue, which sometimes is also named map_value or value_map. Likewise for vOrderForm. Sure we can have that comment documenting what it contains, but comments can become outdated and I think it’s much nicer if the code itself can be self-documenting.

The compatibility issues can be trivially resolved if that’s desired, but I don’t think there will be any compatibility issues as new fields are way easier to serialize at the end of the record than converting to a string and inserting into a map.

DrahtBot added the label Needs rebase on Jul 1, 2025

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.

72a01ce799

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.

b5547af6ac

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.

66d18de71c

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

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

389e1094c2

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

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

47506ef609

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.

639019f761

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.

8ec85c6d92

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.

3d2a9ab927

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.

9a8fd16e67

achow101 force-pushed on Jul 1, 2025

DrahtBot removed the label Needs rebase on Jul 1, 2025

w0xlt commented at 7:51 pm on July 1, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/32763/commits/9a8fd16e67755e336de683c0158d4fdd2b4ddfe8. value_map and order_form (and therefore WalletOrderForm and WalletValueMap) lack clarity, making replacing with well-defined fields a much better approach.

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

Code Coverage & Benchmarks

Reviews

Conflicts