Page MenuHomePhabricator

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

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

Details

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
Branch
patch1
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5748
Build 9558: Bitcoin ABC Buildbot (legacy)
Build 9557: arc lint + arc unit

Event Timeline

markblundeberg edited the summary of this revision. (Show Details)EditedMay 8 2019, 02:12

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

jasonbcox requested changes to this revision.May 8 2019, 19:33

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

This is fine since we're mass-backporting atm, but these keys will likely be removed since we've stripped out all bumpfee functionality.

This revision now requires changes to proceed.May 8 2019, 19:33

thanks @jasonbcox good points, I've updated description accordingly

This revision is now accepted and ready to land.May 9 2019, 00:31