wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members #32763
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32763. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
1895122 to
398b632
Compare
|
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? |
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 It would be straightforward to preserve The primary motivation for this change is actually that I am working on a completely new storage mechanism for
Currently they would be stripped out. Updated the description. |
|
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 |
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 |
398b632 to
f1afd3b
Compare
|
Concept NACK, agree with @ryanofsky's concerns. 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 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. |
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.
Instead of passing these by setting them in mapValue, pass them directly to CommitTransaction. This is preparation for removing mapValue.
The values previously passed in mapValue are now parameters to CommitTransaction so there is no need for mapValue to be passed.
Instead of storing "from" and "message" inside of mapValue, store these explicitly as members of CWalletTx.
Instead of storing "comment" and "to" inside of mapValue, store these expliclty as members of CWalletTx.
…variables Instead of storing "replaces_txid" and "replaced_by_txid" as strings inside of mapValue, store these expliclty as members of 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.
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.
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.
f1afd3b to
9a8fd16
Compare
mapValueandvOrderFormare 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
CWalletTxdepends onmapValueandvOrderForm, the serialization remains unchanged, so when serializing these new members, they need to be shoved/extracted from a temporarymapValueorvOrderForm.This does end up breaking forwards compatibility as unknown fields in
mapValueandvOrderFormare 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.