Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Show scanning details in getwalletinfo #15730

Merged
merged 4 commits into from May 6, 2019

Conversation

@promag
Copy link
Member

commented Apr 3, 2019

Closes #15724.

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 514ec2a to ae0da1a Apr 3, 2019

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

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.

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch 2 times, most recently from 8a246c2 to 0981088 Apr 4, 2019

Show resolved Hide resolved src/wallet/wallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 0981088 to 3ae04b2 Apr 14, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2019

Thanks @meshcollider, updated.

@meshcollider

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

utACK 3ae04b2

@MarcoFalke
Copy link
Member

left a comment

utACK 3ae04b2

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 3ae04b2 to 35c8449 Apr 15, 2019

@@ -859,6 +862,8 @@ class CWallet final : public CCryptoKeyStore, private interfaces::Chain::Notific
void AbortRescan() { fAbortRescan = true; }
bool IsAbortingRescan() { return fAbortRescan; }
bool IsScanning() { return fScanningWallet; }
int64_t ScanningDuration() const { return fScanningWallet ? GetTimeMillis() - m_scanning_start : 0; }

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Apr 16, 2019

Member

shouldn't this (in theory) lock mutexScanning to avoid the risk of an out-of-sync fScanningWallet/m_scanning_start combination (same for line below and wallet.cpp:L1795)?

This comment has been minimized.

Copy link
@promag

promag Apr 16, 2019

Author Member

m_scanning_start is constant right before fScanningWallet is set to true so in this case I don't think it's necessary.

The atomic m_scanning_progress is only read when fScanningWallet is true. Also, m_scanning_progress is only changed in ScanForWalletTransactions, and we ensure there's only one scan in progress.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Nice addition!
Concept ACK

Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 35c8449 to 1bf158e Apr 16, 2019

@jonatack
Copy link
Contributor

left a comment

Concept ACK.

Show resolved Hide resolved src/wallet/rpcwallet.cpp
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
Show resolved Hide resolved src/wallet/rpcwallet.cpp Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 22, 2019

Needs rebase, I think this is ready

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 1bf158e to c8664bb Apr 22, 2019

@DrahtBot DrahtBot removed the Needs rebase label Apr 22, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@MarcoFalke rebased.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

utACK c8664bb

@MarcoFalke MarcoFalke added this to the 0.19.0 milestone Apr 23, 2019

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch 2 times, most recently from fd2b32a to f1f2e09 Apr 26, 2019

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from f1f2e09 to 4f9adf4 Apr 28, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Apr 28, 2019

Rebased due to #15901.

Show resolved Hide resolved src/wallet/wallet.cpp Outdated

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 4f9adf4 to 0088966 Apr 28, 2019

Show resolved Hide resolved src/wallet/wallet.cpp Outdated
@jonatack

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Partial tACK 0088966 using testnet. The explicitness of "scanning": false seems 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

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Currently 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

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@jonasschnelli That would absolutely make sense from a usability perspective -- we should do that :-)

@luke-jr

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Nah, JSON-RPC is intended for software, not humans.

@jonatack

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@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 (comment)

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Oh. I missed that comment.
Yeah, @MarcoFalke is right, we use the same in getblockchaininfo verificationprogress which is (in both cases) a double converted to the JSON internal string with std::setprecision(16)

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Tested ACK 0088966

@jonatack

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Mainnet Example:

"scanning": {
    "duration": 2,
    "progress": 8.107329471549215e-07
  }

@jonasschnelli IIUC progress rshould return a value between 0 and 1?

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 2, 2019

@jonatack

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@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 :)

@promag promag force-pushed the promag:2019-04-getwalletinfo-scanning branch from 0088966 to b6c748f May 2, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

Sorry for the force push, had to fix @luke-jr comment #15730 (comment). No changes since 0088966, should be easy to re-ACK.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 2, 2019

re-utACK b6c748f (Only change since my last review is rebase, adding release notes, and returning false instead of null)

Show signature and timestamp

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 -

@jonatack
Copy link
Contributor

left a comment

ACK b6c748f, only changes appear to be rebase for #15730 (comment) 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.

RPC changes
-----------
The RPC `getwalletinfo` response now includes the `scanning` key with an object
if there is a scanning in progress or `false` otherwise. Currently the object

This comment has been minimized.

Copy link
@jonatack

jonatack May 5, 2019

Contributor

s/scanning/scan/

This comment has been minimized.

Copy link
@jonatack

jonatack May 5, 2019

Contributor

better yet: "if a scan is in progress,"

@@ -2441,6 +2446,14 @@ static UniValue getwalletinfo(const JSONRPCRequest& request)
obj.pushKV("hdseedid", seed_id.GetHex());
}
obj.pushKV("private_keys_enabled", !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));
if (pwallet->IsScanning()) {
UniValue scanning(UniValue::VOBJ);
scanning.pushKV("duration", pwallet->ScanningDuration() / 1000);

This comment has been minimized.

Copy link
@jonatack

jonatack May 5, 2019

Contributor

Perhaps divide by 1000 in src/wallet/wallet.h:L825 instead of here?

if (pwallet->IsScanning()) {
UniValue scanning(UniValue::VOBJ);
scanning.pushKV("duration", pwallet->ScanningDuration() / 1000);
scanning.pushKV("progress", pwallet->ScanningProgress());

This comment has been minimized.

Copy link
@jonatack

jonatack May 5, 2019

Contributor

The noun is "scan", so it could be shorter to name these ScanDuration and ScanProgress.

@promag

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@jonatack thanks for your review, will take into account if more changes are necessary.

@laanwj

This comment has been minimized.

Copy link
Member

commented May 6, 2019

utACK b6c748f

going to merge b6c748f, 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 laanwj merged commit b6c748f into bitcoin:master May 6, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 6, 2019

Merge #15730: rpc: Show scanning details in getwalletinfo
b6c748f doc: Add release notes for 15730 (João Barbosa)
d3e8458 rpc: Show scanning details in getwalletinfo (João Barbosa)
90e27ab wallet: Track current scanning progress (João Barbosa)
2ee811e wallet: Track scanning duration (João Barbosa)

Pull request description:

  Closes #15724.

ACKs for commit b6c748:
  MarcoFalke:
    re-utACK b6c748f (Only change since my last review is rebase, adding release notes, and returning false instead of null)
  laanwj:
    utACK b6c748f
  jonatack:
    ACK b6c748f, only changes appear to be rebase for #15730 (comment) and release notes.

Tree-SHA512: 8ee98f971c15f66ce8138fc92c55e51abc9faf01866a31ac7ce2ad766aa2bb88559eabee3b5815d645c84cdf1c19dc35ec03f31461e39bc5f6040edec0b87116

@promag promag deleted the promag:2019-04-getwalletinfo-scanning branch May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.