Differential D7718

[backport#17283] rpc: improve getaddressinfo test coverage, help, code docs
ClosedPublic
Actions

Authored by majcosta on Oct 1 2020, 16:30.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCcc5951bf2a3b: [backport#17283] rpc: improve getaddressinfo test coverage, help, code docs

Summary

33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151 test: add rpc getaddressinfo labels test coverage (Jon Atack)
0f3539ac6d772fc646b5f184fa1efe77bf632f6a test: add listlabels test in wallet_labels.py (Jon Atack)
1388de83900eaced906d369fe9e8887ae74b2dcf rpc: add getaddressinfo code documentation (Jon Atack)
2ee0cb3330ccf70f0540cb42370796e32eff1569 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack)
8d1ed0c263f8cdff7189f02040b5d02238d93da0 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack)
5a0ed850700dfb19167d40b38f80313bd5e427ca rpc: improve getaddressinfo RPCHelpman content (Jon Atack)
70cda342cd20d0e0cd9f28405457544036968f2d rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack)

Pull request description:

This PR is a continuation of the work in https://github.com/bitcoin/bitcoin/pull/12892.

Main motivations:
- There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address.
- `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers.

Changes by order of commits:
- [x] improve/fix getaddressinfo RPCHelpman layout formatting
- [x] improve/fix getaddressinfo RPCHelpman content
- [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman
- [x] update getaddressinfo RPCExamples addresses to bech32
- [x] add getaddressinfo code docs
- [x] add a `listlabels` test assertion in wallet_labels.py
- [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests

Here are gists of the CLI help output:
[`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810)
[`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba)

It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._

Depends on D7716

Backport of Core PR17283

Test Plan

ninja all check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr17283

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 13126
Build 26302: Build Diff	lint-circular-dependencies · build-without-wallet · build-diff · build-clang-tidy · build-clang
Build 26301: arc lint + arc unit

Event Timeline

majcosta created this revision.Oct 1 2020, 16:30

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 1 2020, 16:30

majcosta requested review of this revision.Oct 1 2020, 16:30

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

Harbormaster completed remote builds in B13126: Diff 24153.Oct 1 2020, 16:39

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Snippet of first build failure:

[319/482] Building CXX object src/CMakeFiles/script.dir/script/standard.cpp.o
[320/482] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[321/482] Building CXX object src/CMakeFiles/script.dir/script/sign.cpp.o
[322/482] Linking C static library src/secp256k1/libsecp256k1.a
[323/482] Building CXX object src/CMakeFiles/bitcoin-cli.dir/bitcoin-cli.cpp.o
[324/482] Linking C executable src/secp256k1/ecmult-bench
[325/482] Linking C executable src/secp256k1/internal-bench
[326/482] Linking C executable src/secp256k1/verify-bench
[327/482] Linking C executable src/secp256k1/sign-bench
[328/482] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[329/482] Linking C executable src/secp256k1/recover-bench
[330/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[331/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[332/482] Building CXX object src/CMakeFiles/script.dir/script/signingprovider.cpp.o
[333/482] Building CXX object src/CMakeFiles/bitcoin-wallet.dir/bitcoin-wallet.cpp.o
[334/482] Linking CXX static library src/libscript.a
[335/482] Linking CXX static library src/libcommon.a
[336/482] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[337/482] Linking CXX executable src/bitcoin-cli
[338/482] Linking CXX static library src/libbitcoinconsensus.a
[339/482] Linking CXX shared library src/libbitcoinconsensus.so.0.22.4
[340/482] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[341/482] Linking CXX executable src/bitcoin-tx
[342/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/blockfilter.cpp.o
[343/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[344/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[345/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[346/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[347/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/db.cpp.o 
/usr/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy-8;-warnings-as-errors=*" --source=../../src/wallet/db.cpp -- /usr/bin/clang++  -DBOOST_AC_USE_STD_ATOMIC -DBOOST_SP_USE_STD_ATOMIC -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -isystem /usr/include/jemalloc -g -O2 -fPIC -fvisibility=hidden   -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety-analysis -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++14 -MD -MT src/wallet/CMakeFiles/wallet.dir/db.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/db.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/db.cpp.o -c ../../src/wallet/db.cpp
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/db.cpp:869:30: error: statement should be inside braces [readability-braces-around-statements,-warnings-as-errors]
                if (!fMockDb) dbenv->lsn_reset(strFile.c_str(), 0);
                             ^
                              {
2458 warnings generated.
Suppressed 2457 warnings (2457 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
[348/482] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[349/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[350/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[351/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/dns.cpp.o
[352/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[353/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/bitcoin.cpp.o
[354/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[355/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[356/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/db.cpp.o
[357/482] Linking CXX static library src/seeder/libseeder.a
[358/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[359/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/psbtwallet.cpp.o
[360/482] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[361/482] Linking CXX executable src/seeder/bitcoin-seeder
[362/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[363/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[364/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[365/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[366/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

oh, left some segwit stuff in there, fixing it

removed segwit crud

Harbormaster completed remote builds in B13132: Diff 24161.Oct 1 2020, 17:39

Build Bitcoin ABC Diffs / Diff Testing (build-clang-tidy) failed.

Snippet of first build failure:

[319/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/logging.cpp.o
[320/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/transaction_utils.cpp.o
[321/482] Building C object src/secp256k1/CMakeFiles/secp256k1.dir/src/secp256k1.c.o
[322/482] Linking C static library src/secp256k1/libsecp256k1.a
[323/482] Linking C executable src/secp256k1/ecmult-bench
[324/482] Linking C executable src/secp256k1/internal-bench
[325/482] Linking C executable src/secp256k1/sign-bench
[326/482] Linking C executable src/secp256k1/verify-bench
[327/482] Linking C executable src/secp256k1/recover-bench
[328/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/mining.cpp.o
[329/482] Building CXX object src/CMakeFiles/script.dir/script/descriptor.cpp.o
[330/482] Linking CXX static library src/libscript.a
[331/482] Linking CXX static library src/libcommon.a
[332/482] Linking CXX executable src/bitcoin-cli
[333/482] Linking CXX static library src/libbitcoinconsensus.a
[334/482] Linking CXX shared library src/libbitcoinconsensus.so.0.22.4
[335/482] Creating library symlink src/libbitcoinconsensus.so.0 src/libbitcoinconsensus.so
[336/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/db.cpp.o
FAILED: src/wallet/CMakeFiles/wallet.dir/db.cpp.o 
/usr/bin/cmake -E __run_co_compile --launcher=/usr/bin/ccache --tidy="/usr/bin/clang-tidy-8;-warnings-as-errors=*" --source=../../src/wallet/db.cpp -- /usr/bin/clang++  -DBOOST_AC_USE_STD_ATOMIC -DBOOST_SP_USE_STD_ATOMIC -DBUILD_BITCOIN_INTERNAL -DENABLE_AVX2 -DENABLE_SHANI -DENABLE_SSE41 -DHAVE_BUILD_INFO -DHAVE_CONFIG_H -DHAVE_CONSENSUS_LIB -I../../src/. -Isrc -I../../src/univalue/include -Isrc/crypto/.. -I../../src/secp256k1/include -isystem /usr/include/jemalloc -g -O2 -fPIC -fvisibility=hidden   -fstack-protector-all -Wstack-protector -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wextra -Wformat -Wvla -Wcast-align -Wunused-parameter -Wmissing-braces -Wthread-safety-analysis -Wshadow -Wshadow-field -Wrange-loop-analysis -Wredundant-decls -Wformat-security -Wredundant-move -Wno-unused-parameter -Wno-implicit-fallthrough -pthread -std=gnu++14 -MD -MT src/wallet/CMakeFiles/wallet.dir/db.cpp.o -MF src/wallet/CMakeFiles/wallet.dir/db.cpp.o.d -o src/wallet/CMakeFiles/wallet.dir/db.cpp.o -c ../../src/wallet/db.cpp
/work/abc-ci-builds/build-clang-tidy/../../src/wallet/db.cpp:869:30: error: statement should be inside braces [readability-braces-around-statements,-warnings-as-errors]
                if (!fMockDb) dbenv->lsn_reset(strFile.c_str(), 0);
                             ^
                              {
2458 warnings generated.
Suppressed 2457 warnings (2457 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
1 warning treated as error
[337/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coincontrol.cpp.o
[338/482] Building CXX object src/CMakeFiles/server.dir/validation.cpp.o
[339/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/crypter.cpp.o
[340/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/coinselection.cpp.o
[341/482] Building CXX object src/CMakeFiles/bitcoin-tx.dir/bitcoin-tx.cpp.o
[342/482] Linking CXX executable src/bitcoin-tx
[343/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/wallet.cpp.o
[344/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqabstractnotifier.cpp.o
[345/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/fees.cpp.o
[346/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/psbtwallet.cpp.o
[347/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletutil.cpp.o
[348/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/load.cpp.o
[349/482] Building CXX object src/test/CMakeFiles/testutil.dir/util/setup_common.cpp.o
[350/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/dns.cpp.o
[351/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/db.cpp.o
[352/482] Building CXX object src/wallet/CMakeFiles/wallet-tool.dir/wallettool.cpp.o
[353/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqnotificationinterface.cpp.o
[354/482] Building CXX object src/seeder/CMakeFiles/seeder.dir/bitcoin.cpp.o
[355/482] Linking CXX static library src/seeder/libseeder.a
[356/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/__/interfaces/wallet.cpp.o
[357/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqpublishnotifier.cpp.o
[358/482] Building CXX object src/seeder/CMakeFiles/bitcoin-seeder.dir/main.cpp.o
[359/482] Linking CXX executable src/seeder/bitcoin-seeder
[360/482] Building CXX object src/zmq/CMakeFiles/zmq.dir/zmqrpc.cpp.o
[361/482] Linking CXX static library src/zmq/libzmq.a
[362/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/walletdb.cpp.o
[363/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/scriptpubkeyman.cpp.o
[364/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcdump.cpp.o
[365/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/wallet.cpp.o
[366/482] Building CXX object src/wallet/CMakeFiles/wallet.dir/rpcwallet.cpp.o
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang-tidy failed with exit code 1

rebase

majcosta added a child revision: D7724: [backport#17578] rpc: simplify getaddressinfo labels, deprecate previous behavior.Oct 1 2020, 18:28

Harbormaster completed remote builds in B13134: Diff 24165.Oct 1 2020, 18:32

Fabien accepted this revision.Oct 1 2020, 18:49

This revision is now accepted and ready to land.Oct 1 2020, 18:49

Closed by commit rABCcc5951bf2a3b: [backport#17283] rpc: improve getaddressinfo test coverage, help, code docs (authored by Wladimir J. van der Laan <laanwj@protonmail.com>, committed by majcosta). · Explain WhyOct 1 2020, 20:26

This revision was automatically updated to reflect the committed changes.

majcosta added a commit: rABCcc5951bf2a3b: [backport#17283] rpc: improve getaddressinfo test coverage, help, code docs.