Page MenuHomePhabricator

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

Authored by markblundeberg on Wed, May 8, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Wed, May 8, 02:11
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, May 8, 02:11
markblundeberg edited the summary of this revision. (Show Details)EditedWed, May 8, 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.Wed, May 8, 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 ↗(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.

This revision now requires changes to proceed.Wed, May 8, 19:33
markblundeberg edited the test plan for this revision. (Show Details)Wed, May 8, 23:47
markblundeberg requested review of this revision.Wed, May 8, 23:51

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

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