Merge #9333: Document CWalletTx::mapValue entries and remove erase of nonexistent "version" entry.
ClosedPublic
Actions

Authored by markblundeberg on May 8 2019, 02:11.

Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Project

Commits

rSTAGINGfd8c2df78616: Merge #9333: Document CWalletTx::mapValue entries and remove erase of…
rABCfd8c2df78616: Merge #9333: Document CWalletTx::mapValue entries and remove erase of…

Summary

backport PR9333 https://github.com/bitcoin/bitcoin/pull/9333/files
87ed396 [trivial] Add comment documenting bumpfee mapValues (Russell Yanofsky)
a1fe944 Remove reference to nonexistent "version" wallet transaction mapvalue field (Russell Yanofsky)
654e044 [trivial] Add comment documenting CWalletTx::mapValue (Russell Yanofsky)

Tree-SHA512: 1fd1860e345c59b13634db2007fff4ba30aaf1f177fdd765f47bf9257fac117cdcd5d491424416da304c08e85effbb27f3424f072f7c9587ef39cb98531b932a

Test Plan

make check
test-runner.py

note, as mentioned by a1fe944 commit message, the line about "version" was introduced by accident in olden times (865c3a2) and the "version" field never actually got used (and it's certainly mentioned nowhere else in codebase)

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markblundeberg created this revision.May 8 2019, 02:11

Herald added a reviewer: Restricted Project. · View Herald TranscriptMay 8 2019, 02:11

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Looks like replaces_txid is no longer used but replaced_by_txid has one mention in codebase. (wallet.cpp)

Harbormaster completed remote builds in B5748: Diff 8491.May 8 2019, 02:26

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

markblundeberg added a reviewer: jasonbcox.May 8 2019, 18:50

The mapValue.erase("version"); is clearly a code change and requires some sort of testing. A make check + test_runner.py would be sufficient if tests cover this area. Fortunately, the tests have been run for you, but do make sure to mention that this is the intention in the test plan.

Another sanity check would be to make sure there are no missing backports prior to this that cleanup uses of "version". You may also indicate in the test plan that a grep for uses of "version" as a key in the wallet turn up no results.

src/wallet/wallet.h
283 ↗	(On Diff #8491)	This is fine since we're mass-backporting atm, but these keys will likely be removed since we've stripped out all bumpfee functionality.