Closes #15724.
rpc: Show scanning details in getwalletinfo #15730
pull promag wants to merge 4 commits into bitcoin:master from promag:2019-04-getwalletinfo-scanning changing 4 files +26 −1-
promag commented at 4:09 PM on April 3, 2019: member
- promag force-pushed on Apr 3, 2019
-
in src/wallet/rpcwallet.cpp:2410 in ae0da1a789 outdated
2405 | @@ -2406,6 +2406,9 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2406 | " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" 2407 | " \"hdseedid\": \"<hash160>\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" 2408 | " \"private_keys_enabled\": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)\n" 2409 | + " \"scannning_active\": true|false (boolean) whether the local blockchain is being scanned for wallet related transactions\n" 2410 | + " \"scanning_duration\": true|false (numeric) scanning duration in milliseconds\n"
promag commented at 4:12 PM on April 3, 2019:not boolean..
in src/wallet/rpcwallet.cpp:2409 in ae0da1a789 outdated
2405 | @@ -2406,6 +2406,9 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2406 | " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" 2407 | " \"hdseedid\": \"<hash160>\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" 2408 | " \"private_keys_enabled\": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)\n" 2409 | + " \"scannning_active\": true|false (boolean) whether the local blockchain is being scanned for wallet related transactions\n"
MarcoFalke commented at 4:13 PM on April 3, 2019:Could make it
scanning: Noneorscanning: { 'duration': (num), 'progress': (num), }
promag commented at 4:15 PM on April 3, 2019:Yap, I'm fine either way.
MarcoFalke commented at 4:17 PM on April 3, 2019:Indeed, just a style-preference. Though, json provides structure, so might as well use it instead of returning a single huge dict
promag commented at 9:17 AM on April 4, 2019:Done.
DrahtBot commented at 5:07 PM on April 3, 2019: member<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #14942 (wallet: Make scan / abort status private to CWallet by Empact)
- #13756 (wallet: "avoid_reuse" wallet flag for improved privacy by kallewoof)
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.
fanquake added the label RPC/REST/ZMQ on Apr 3, 2019promag force-pushed on Apr 4, 2019promag force-pushed on Apr 4, 2019in src/wallet/wallet.cpp:1801 in 27490614a1 outdated
1791 | @@ -1792,8 +1792,9 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc 1792 | } 1793 | double progress_current = progress_begin; 1794 | while (block_height && !fAbortRescan && !chain().shutdownRequested()) { 1795 | + m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
meshcollider commented at 3:07 AM on April 14, 2019:Can we ensure
progress_end - progress_begin != 0at this point?in src/wallet/rpcwallet.cpp:2405 in 098108848e outdated
2405 | @@ -2406,6 +2406,11 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2406 | " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" 2407 | " \"hdseedid\": \"<hash160>\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" 2408 | " \"private_keys_enabled\": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)\n" 2409 | + " \"scanning\": (json objects) current scanning details, or null if no scan is in progress\n"
meshcollider commented at 3:10 AM on April 14, 2019:nit:
objects->object
luke-jr commented at 8:31 PM on April 20, 2019:Just one object, right?
promag commented at 8:35 PM on April 20, 2019:Right!
in src/wallet/rpcwallet.cpp:2453 in 098108848e outdated
2448 | @@ -2444,6 +2449,14 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2449 | obj.pushKV("hdseedid", seed_id.GetHex()); 2450 | } 2451 | obj.pushKV("private_keys_enabled", !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); 2452 | + if (pwallet->IsScanning()) { 2453 | + UniValue scanning(UniValue::VOBJ);
meshcollider commented at 3:46 AM on April 14, 2019:nit: indentation is wrong here
promag force-pushed on Apr 14, 2019promag commented at 9:54 AM on April 14, 2019: memberThanks @meshcollider, updated.
meshcollider commented at 8:13 PM on April 14, 2019: contributorin src/wallet/rpcwallet.cpp:2411 in 3ae04b2834 outdated
2405 | @@ -2406,6 +2406,11 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2406 | " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" 2407 | " \"hdseedid\": \"<hash160>\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" 2408 | " \"private_keys_enabled\": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)\n" 2409 | + " \"scanning\": (json object) current scanning details, or null if no scan is in progress\n" 2410 | + " {\n" 2411 | + " \"duration\" : xxxx (numeric) elapsed milliseconds since scan start\n"
MarcoFalke commented at 7:15 PM on April 15, 2019:This should probably be seconds. I doubt that a user is interested in knowing the exact milliseconds
promag commented at 9:02 PM on April 15, 2019:Yeah but I thought
"duration": 1.23seconds is worst because of the decimal part. Happy to change though.
MarcoFalke commented at 9:08 PM on April 15, 2019:Heh, could truncate
promag commented at 9:47 PM on April 15, 2019:Changed to seconds, only integral part.
MarcoFalke approvedMarcoFalke commented at 7:16 PM on April 15, 2019: memberutACK 3ae04b283453b669bcb0809380e973aa7e7adac0
promag force-pushed on Apr 15, 2019in src/wallet/wallet.h:825 in 35c8449063 outdated
861 | @@ -859,6 +862,8 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific 862 | void AbortRescan() { fAbortRescan = true; } 863 | bool IsAbortingRescan() { return fAbortRescan; } 864 | bool IsScanning() { return fScanningWallet; } 865 | + int64_t ScanningDuration() const { return fScanningWallet ? GetTimeMillis() - m_scanning_start : 0; }
jonasschnelli commented at 7:45 AM on April 16, 2019:shouldn't this (in theory) lock
mutexScanningto avoid the risk of an out-of-syncfScanningWallet/m_scanning_startcombination (same for line below and wallet.cpp:L1795)?
promag commented at 1:35 PM on April 16, 2019:m_scanning_startis constant right beforefScanningWalletis set totrueso in this case I don't think it's necessary.The atomic
m_scanning_progressis only read whenfScanningWalletistrue. Also,m_scanning_progressis only changed inScanForWalletTransactions, and we ensure there's only one scan in progress.jonasschnelli commented at 7:46 AM on April 16, 2019: contributorNice addition! Concept ACK
in src/wallet/rpcwallet.cpp:2458 in 35c8449063 outdated
2453 | + UniValue scanning(UniValue::VOBJ); 2454 | + scanning.pushKV("duration", pwallet->ScanningDuration() / 1000); 2455 | + scanning.pushKV("progress", pwallet->ScanningProgress()); 2456 | + obj.pushKV("scanning", scanning); 2457 | + } else { 2458 | + obj.pushKV("scanning", UniValue());
MarcoFalke commented at 11:52 AM on April 16, 2019:obj.pushKV("scanning", UniValue());https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy
promag commented at 2:22 PM on April 16, 2019:Fixed.
promag force-pushed on Apr 16, 2019in src/wallet/rpcwallet.cpp:2410 in 1bf158e422 outdated
2405 | @@ -2406,6 +2406,11 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2406 | " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" 2407 | " \"hdseedid\": \"<hash160>\" (string, optional) the Hash160 of the HD seed (only present when HD is enabled)\n" 2408 | " \"private_keys_enabled\": true|false (boolean) false if privatekeys are disabled for this wallet (enforced watch-only wallet)\n" 2409 | + " \"scanning\": (json objects) current scanning details, or null if no scan is in progress\n" 2410 | + " {\n" 2411 | + " \"duration\" : xxxx (numeric) elapsed seconds since scan start\n" 2412 | + " \"progress\" : x.xxxx, (numeric) scanning progress percentage [0.0, 1.0]\n" 2413 | + " }\n" 2414 | "}\n"
jonatack commented at 2:33 PM on April 16, 2019:Nice! Am indeed seeing (in testnet):
"scanning": { "duration": 168, "progress": 0.265890921329112 }However, after running
abortrescan, this is output:"scanning": nullIs this the desired behavior?
Note that in these 2 cases, the field is absent. EDIT: No scanning key returned in those cases during my manual testnet testing.
- before rescanblockchain
- after abortrescan, stopping, and restarting bitcoind
jonatack commented at 2:38 PM on April 16, 2019:(Might be good to add a test that defines the desired behavior, and describe it in the commit message).
promag commented at 2:40 PM on April 16, 2019:Is this the desired behavior?
Yes, this was suggested by @MarcoFalke.
scanningkey is also present right?BTW,
"progress": 0.265890921329112maybe should limit precision?
promag commented at 2:42 PM on April 16, 2019:(Might be good to add a test that defines the desired behavior, and describe it in the commit message).
I can test the key is there, but I don't think there's a reliable way to test while rescanning.
MarcoFalke commented at 3:17 PM on April 16, 2019:Precision is fine. We use the same for
"verificationprogress": 0.9831896495042335,
MarcoFalke commented at 3:18 PM on April 16, 2019:No scanning key returned in those cases during my manual testnet testing.
Are you sure you are running the right binary? All code paths should push back a "scanning" key?
jonatack commented at 3:38 PM on April 16, 2019:Are you sure you are running the right binary? All code paths should push back a "scanning" key?
Argh, you are right about the binary. Now seeing
"scanning": nullin the cases above. Makes sense givensrc/wallet/rpcwallet.cpp:2458. Apologies.jonatack commented at 2:35 PM on April 16, 2019: memberConcept ACK.
DrahtBot added the label Needs rebase on Apr 17, 2019in src/wallet/rpcwallet.cpp:2455 in 1bf158e422 outdated
2453 | + UniValue scanning(UniValue::VOBJ); 2454 | + scanning.pushKV("duration", pwallet->ScanningDuration() / 1000); 2455 | + scanning.pushKV("progress", pwallet->ScanningProgress()); 2456 | + obj.pushKV("scanning", scanning); 2457 | + } else { 2458 | + obj.pushKV("scanning", UniValue());
luke-jr commented at 6:19 AM on April 18, 2019:Seems better to just omit it?
promag commented at 6:38 AM on April 18, 2019:Yeah I could do that.
MarcoFalke commented at 2:12 PM on April 22, 2019:Then the result would look the same as in the previous version and you couldn't tell (without looking at the bitcoin core version) whether a rescan is happening or not.
promag commented at 2:07 PM on April 26, 2019:Yeap, that's true if the client is human.
luke-jr commented at 5:11 PM on April 26, 2019:IIRC
nullis the same as omitted in JSON-RPC, so maybe in that case we should putfalsehere?
MarcoFalke commented at 5:15 PM on April 26, 2019:yeah, the value is omitted, but not the key.
Though, I am also fine with "false", maybe it is more clear even
promag commented at 10:05 AM on April 28, 2019:Changed
nulltofalse.MarcoFalke commented at 2:13 PM on April 22, 2019: memberNeeds rebase, I think this is ready
promag force-pushed on Apr 22, 2019DrahtBot removed the label Needs rebase on Apr 22, 2019promag commented at 12:17 AM on April 23, 2019: member@MarcoFalke rebased.
MarcoFalke commented at 11:52 AM on April 23, 2019: memberutACK c8664bb0c76f0c6a1eedaa253a03640b5dba7d38
MarcoFalke added this to the milestone 0.19.0 on Apr 23, 2019promag force-pushed on Apr 26, 2019promag force-pushed on Apr 28, 2019wallet: Track scanning duration 2ee811e693promag force-pushed on Apr 28, 2019in src/wallet/wallet.cpp:1801 in 4f9adf41a3 outdated
1797 | @@ -1798,8 +1798,9 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc 1798 | } 1799 | double progress_current = progress_begin; 1800 | while (block_height && !fAbortRescan && !chain().shutdownRequested()) { 1801 | + m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin)
MarcoFalke commented at 1:19 PM on April 28, 2019:m_scanning_progress = (progress_current - progress_begin) / (progress_end - progress_begin);
luke-jr commented at 9:07 AM on May 1, 2019:Semicolon is still missing from this commit.
promag commented at 10:41 AM on May 2, 2019:Thanks, bad fixup :/
promag force-pushed on Apr 28, 2019jonatack commented at 12:34 PM on May 1, 2019: memberPartial tACK https://github.com/bitcoin/bitcoin/pull/15730/commits/008896693c0336b27d54833939fc9c65b544a362 using testnet. The explicitness of
"scanning": falseseems to me an improvement over returning null when no rescan is in progress. Interesting that no tests need be updated and might be good to add coverage in this PR or a follow-up. I've written functional test coverage for this as an exercise but am unsure if coverage like this is welcome given the Travis CI timeouts.Output before/after rescan, and during:
"private_keys_enabled": true, "scanning": false }"private_keys_enabled": true, "scanning": { "duration": 35, "progress": 0.04219915475488047 } }jonasschnelli commented at 8:53 AM on May 2, 2019: contributorCurrently testing... Is it intended to output the progress as scientific notation representation? Mainnet Example:
"scanning": { "duration": 2, "progress": 8.107329471549215e-07 }Would it make sense to round it to a certain precision level?
practicalswift commented at 9:11 AM on May 2, 2019: contributor@jonasschnelli That would absolutely make sense from a usability perspective -- we should do that :-)
luke-jr commented at 9:19 AM on May 2, 2019: memberNah, JSON-RPC is intended for software, not humans.
jonatack commented at 9:21 AM on May 2, 2019: member@jonasschnelli I'm not seeing that notation on Linux Debian with mainnet and testnet, is it perhaps implementation-dependent?
There was a discussion about precision above: #15730 (review)
jonasschnelli commented at 9:25 AM on May 2, 2019: contributorOh. I missed that comment. Yeah, @MarcoFalke is right, we use the same in
getblockchaininfoverificationprogresswhich is (in both cases) adoubleconverted to the JSON internal string withstd::setprecision(16)jonasschnelli commented at 9:25 AM on May 2, 2019: contributorTested ACK 008896693c0336b27d54833939fc9c65b544a362
jonatack commented at 9:26 AM on May 2, 2019: memberMainnet Example:
"scanning": { "duration": 2, "progress": 8.107329471549215e-07 }@jonasschnelli IIUC progress rshould return a value between 0 and 1?
jonasschnelli commented at 9:28 AM on May 2, 2019: contributor@jonasschnelli IIUC progress rshould return a value between 0 and 1?
What do you mean with that? 8.107329471549215e-07 is between 0 and 1 (its ~== 0.000000810732947).
practicalswift commented at 9:29 AM on May 2, 2019: contributorjonatack commented at 9:29 AM on May 2, 2019: member@jonasschnelli IIUC progress rshould return a value between 0 and 1?
What do you mean with that? 8.107329471549215e-07 is between 0 and 1 (its ~== 0.000000810732947).
Right. Never mind :)
wallet: Track current scanning progress 90e27abe37rpc: Show scanning details in getwalletinfo d3e8458365doc: Add release notes for 15730 b6c748f849promag force-pushed on May 2, 2019promag commented at 10:41 AM on May 2, 2019: memberSorry for the force push, had to fix @luke-jr comment #15730 (review). No changes since 0088966, should be easy to re-ACK.
MarcoFalke commented at 8:43 PM on May 2, 2019: memberre-utACK b6c748f849 (Only change since my last review is rebase, adding release notes, and returning false instead of null)
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512 re-utACK b6c748f849 (Only change since my last review is rebase, adding release notes, and returning false instead of null) -----BEGIN PGP SIGNATURE----- iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p pUjTfQwAjgYRZzmMw0DsDTsd9Mdtrv2FdtZYDZ2OPveReGdaRr6o3OuOxxZV20I5 H2CC32TXAPNdnKSknKFTE3gM3pkVW+rACqVosPXMTEYRHY8ptba+gd+/4FQcP1w0 uFMtyvArs56oUu1wyr72h65I5saDhtzsrnDt3L0exXpb4bwyxkzQ74QidkyetRKy i3lVBN4m3kQL9Bf5KeNLXz1FFlSTmgmj5F28UqoB7pbkkBsSBXatGz8bpjVSzTUt akHw0e7lkjzm6uKBGMaZJR6T8tLXflNhLV3mQXdaV45CcCoag73M5KmVIVXGKS66 2D607qZ7qoDs5JLhhDs8NpOReI2uFvyYEn4P4lYXwyc1WIM1i4QbHjhOcwtMmsZw B1DgdSriZxvqV8pBEH2fMGkDWiqZA9jFH3ueiEPeZpotLpMr0FNdtZimMamnnnFf pEJEZ+sbewgjl0FIRYw1JPE4vuUaUU9knCOuCabTEbXTzarRRxH6h4TVEzFBxIMJ OMPJdjtJ =P6oE -----END PGP SIGNATURE-----Timestamp of file with hash
39258b732cad140a2392161228d1a4374d9062b0260ba48234d314aec2782c73 -</details>
in doc/release-notes-15730.md:4 in b6c748f849
0 | @@ -0,0 +1,5 @@ 1 | +RPC changes 2 | +----------- 3 | +The RPC `getwalletinfo` response now includes the `scanning` key with an object 4 | +if there is a scanning in progress or `false` otherwise. Currently the object
jonatack commented at 11:34 AM on May 5, 2019:s/scanning/scan/
jonatack commented at 11:41 AM on May 5, 2019:better yet: "if a scan is in progress,"
in src/wallet/rpcwallet.cpp:2451 in b6c748f849
2445 | @@ -2441,6 +2446,14 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2446 | obj.pushKV("hdseedid", seed_id.GetHex()); 2447 | } 2448 | obj.pushKV("private_keys_enabled", !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); 2449 | + if (pwallet->IsScanning()) { 2450 | + UniValue scanning(UniValue::VOBJ); 2451 | + scanning.pushKV("duration", pwallet->ScanningDuration() / 1000);
jonatack commented at 11:36 AM on May 5, 2019:Perhaps divide by 1000 in src/wallet/wallet.h:L825 instead of here?
in src/wallet/rpcwallet.cpp:2452 in b6c748f849
2445 | @@ -2441,6 +2446,14 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) 2446 | obj.pushKV("hdseedid", seed_id.GetHex()); 2447 | } 2448 | obj.pushKV("private_keys_enabled", !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)); 2449 | + if (pwallet->IsScanning()) { 2450 | + UniValue scanning(UniValue::VOBJ); 2451 | + scanning.pushKV("duration", pwallet->ScanningDuration() / 1000); 2452 | + scanning.pushKV("progress", pwallet->ScanningProgress());
jonatack commented at 11:36 AM on May 5, 2019:The noun is "scan", so it could be shorter to name these
ScanDurationandScanProgress.jonatack commented at 11:38 AM on May 5, 2019: memberACK b6c748f84909212dce73e4b77aa125ed1e108a10, only changes appear to be rebase for #15730 (review) and release notes.
Considering that the noun is "scan" (scanning is a present participle verb or a gerund), for brevity some of the naming in the code could be shortened to
Scan.laanwj commented at 11:37 AM on May 6, 2019: memberutACK b6c748f84909212dce73e4b77aa125ed1e108a10
going to merge b6c748f84909212dce73e4b77aa125ed1e108a10, looks like this is ready (or at least ready enough for this early in merging for 0.19) and has been re-ACKed enough times
laanwj merged this on May 6, 2019laanwj closed this on May 6, 2019laanwj referenced this in commit c5ffe8d515 on May 6, 2019promag deleted the branch on May 6, 2019UdjinM6 referenced this in commit b2fcbac8db on Oct 28, 2020UdjinM6 referenced this in commit 0c8a79a2c5 on Oct 29, 2020UdjinM6 referenced this in commit 486b5c3bcf on Nov 3, 2020UdjinM6 referenced this in commit b3bbc00dbc on Nov 3, 2020akshaynexus referenced this in commit d39efc6599 on Nov 23, 2020ckti referenced this in commit 21def6b9b8 on Mar 28, 2021malbit referenced this in commit aedad82dc1 on Nov 5, 2021MarcoFalke locked this on Dec 16, 2021LabelsMilestone
0.19.0
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me