[test] Add test for getmemoryinfo #10257

jimmysong · 2017-04-22T02:08:09Z

Checks memory before and after a transaction that requires a private key.
Each time, 32 bytes of memory for a private key should be used.
Tested in wallet.py instead of its own file to save testing time.

laanwj · 2017-04-25T11:32:23Z

It's good to have a test for getmemoryinfo, but I'm not sure we should hardcode 32 bytes per key in the test. It should be acceptable for the wallet implementation to vary this, there's no reason to force a certain allocation policy.

I guess a requirement "at least 32 bytes of locked memory should be allocated per key" would do? Then check at the end versus the beginning, and not so much whether it gets allocated in 32 byte increments.

jimmysong · 2017-04-25T14:31:52Z

Made changes per @laanwj and squashed.

laanwj · 2017-04-25T14:53:24Z

Thanks!
utACK 32eb3e7

paveljanik · 2017-04-25T16:39:04Z

test/functional/wallet.py

@@ -57,8 +57,12 @@ def run_test (self):
        assert_equal(len(self.nodes[2].listunspent()), 0)

        # Send 21 BTC from 0 to 2 using sendtoaddress call.
+        # Locked memory should use at least 32 bytes to sign each transaction


How does 64 below correspond to 32 bytes here?

Two getnewaddress/sendtoaddress calls?

Yes, two transactions get signed.

OK, but where is it guaranteed that [locked][used] is not re-used or freed after use? You test some other internal implementation behaviour here, not getmemoryinfo.

Good question. @laanwj, are there any guarantees to getmemoryinfo that can be tested? I was under the impression that the locked memory is only used for private keys and thus is really only reserved for wallet. But how do we know it won't get freed?

Wallet keys are never freed (until shutdown), so this should be OK.

paveljanik · 2017-04-28T17:24:41Z

ACK 32eb3e7

jnewbery · 2017-04-28T19:35:20Z

I'm on the fence about this. It's good to increase code coverage and exercise a new RPC, but I'm not sure this is the best way to go about this. getmemoryinfo isn't obviously linked to wallet functionality, so I don't think it naturally lives in the wallet.py test. In this case it probably makes more sense to create a new test (perhaps also exercising the other RPCs in misc.cpp).

Another concern, which I think you've addressed in this case after #10257 (comment) is that tests for getmemoryinfo shouldn't be too prescriptive on returned values. I can imagine this is the kind of thing that could harmlessly change between commits, and we don't want to have a test case that keeps on breaking for no good reason.

jimmysong · 2017-04-28T19:53:54Z

@jnewbery I can write a separate test, but the only way to make getmemoryinfo change is via usage of private keys which can only happen by spending coins from the wallet. In my mind, that doesn't seem to warrant a separate test. Maybe I'm missing something about how this RPC call works? Or if there's some other way to test this information, I'm open to ideas.

jnewbery · 2017-04-28T20:11:14Z

Really this just me being overly picky. I think a test script should have a clearly defined objective (eg "this is the wallet test - it tests functions a user might want to use the wallet for"). Adding random one line tests to the script dilutes that objective.

To be honest, it's not really an issue. If we add any more tests for getmemoryinfo in future, I think they should be separated off into their own test script, but since this is such a small delta, it's probably ok to live in wallet.py for now.

Tested ACK 32eb3e7eb9cf9b242ec0dc097332e2210d86e0a6

Checks memory before and after a transaction that requires a private key. Each time, 32 bytes of memory for a private key should be used. Tested in wallet.py instead of its own file to save testing time.

jimmysong · 2017-05-09T21:49:11Z

Rebased and ready. Thanks @MarcoFalke

paveljanik · 2017-05-15T14:50:24Z

reACK d4668f3

d4668f3 [test] Add test for getmemoryinfo (Jimmy Song) Tree-SHA512: f5285022504f7f3a5d85981c7c424e5cf1156167dbc4209933ea2a699b741e427f4f908f6d49435376c0e23347db24eb1129b74805cbfce5e0b4ce9e48f71fb0

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

fanquake added the Tests label Apr 22, 2017

laanwj added the Wallet label Apr 25, 2017

jimmysong force-pushed the test_getmemoryinfo branch from 0fec574 to 32eb3e7 Compare April 25, 2017 14:31

paveljanik reviewed Apr 25, 2017

View reviewed changes

[test] Add test for getmemoryinfo

d4668f3

Checks memory before and after a transaction that requires a private key. Each time, 32 bytes of memory for a private key should be used. Tested in wallet.py instead of its own file to save testing time.

jimmysong force-pushed the test_getmemoryinfo branch from 32eb3e7 to d4668f3 Compare May 9, 2017 21:48

laanwj merged commit d4668f3 into bitcoin:master May 17, 2017

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[test] Add test for getmemoryinfo #10257

[test] Add test for getmemoryinfo #10257

Uh oh!

jimmysong commented Apr 22, 2017

Uh oh!

laanwj commented Apr 25, 2017 •

edited

Loading

Uh oh!

jimmysong commented Apr 25, 2017

Uh oh!

laanwj commented Apr 25, 2017

Uh oh!

paveljanik Apr 25, 2017

Uh oh!

paveljanik Apr 25, 2017 •

edited

Loading

Uh oh!

jimmysong Apr 25, 2017

Uh oh!

paveljanik Apr 25, 2017

Uh oh!

jimmysong Apr 25, 2017

Uh oh!

laanwj Apr 27, 2017 •

edited

Loading

Uh oh!

paveljanik commented Apr 28, 2017

Uh oh!

jnewbery commented Apr 28, 2017

Uh oh!

jimmysong commented Apr 28, 2017 •

edited

Loading

Uh oh!

jnewbery commented Apr 28, 2017 •

edited

Loading

Uh oh!

jimmysong commented May 9, 2017

Uh oh!

paveljanik commented May 15, 2017

Uh oh!

Uh oh!

[test] Add test for getmemoryinfo #10257

[test] Add test for getmemoryinfo #10257

Uh oh!

Conversation

jimmysong commented Apr 22, 2017

Uh oh!

laanwj commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmysong commented Apr 25, 2017

Uh oh!

laanwj commented Apr 25, 2017

Uh oh!

paveljanik Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmysong Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

jimmysong Apr 25, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Apr 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Apr 28, 2017

Uh oh!

jnewbery commented Apr 28, 2017

Uh oh!

jimmysong commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmysong commented May 9, 2017

Uh oh!

paveljanik commented May 15, 2017

Uh oh!

Uh oh!

laanwj commented Apr 25, 2017 •

edited

Loading

paveljanik Apr 25, 2017 •

edited

Loading

laanwj Apr 27, 2017 •

edited

Loading

jimmysong commented Apr 28, 2017 •

edited

Loading

jnewbery commented Apr 28, 2017 •

edited

Loading