[test] Add test for getmemoryinfo #10257

jimmysong commented at 2:08 AM on April 22, 2017: contributor

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.

fanquake added the label Tests on Apr 22, 2017

laanwj commented at 11:32 AM on April 25, 2017: member

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.

laanwj added the label Wallet on Apr 25, 2017

jimmysong force-pushed on Apr 25, 2017

jimmysong commented at 2:31 PM on April 25, 2017: contributor

Made changes per @laanwj and squashed.

laanwj commented at 2:53 PM on April 25, 2017: member

Thanks! utACK https://github.com/bitcoin/bitcoin/pull/10257/commits/32eb3e7eb9cf9b242ec0dc097332e2210d86e0a6

in test/functional/wallet.py:60 in 32eb3e7eb9 outdated

  56 | @@ -57,8 +57,12 @@ def run_test (self):
  57 |          assert_equal(len(self.nodes[2].listunspent()), 0)
  58 |  
  59 |          # Send 21 BTC from 0 to 2 using sendtoaddress call.
  60 | +        # Locked memory should use at least 32 bytes to sign each transaction

paveljanik commented at 4:39 PM on April 25, 2017:

How does 64 below correspond to 32 bytes here?

paveljanik commented at 4:39 PM on April 25, 2017:

Two getnewaddress/sendtoaddress calls?

jimmysong commented at 5:00 PM on April 25, 2017:

Yes, two transactions get signed.

paveljanik commented at 5:03 PM on April 25, 2017:

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.

jimmysong commented at 5:08 PM on April 25, 2017:

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?

laanwj commented at 2:00 PM on April 27, 2017:

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

paveljanik commented at 5:24 PM on April 28, 2017: contributor

ACK https://github.com/bitcoin/bitcoin/pull/10257/commits/32eb3e7eb9cf9b242ec0dc097332e2210d86e0a6

jnewbery commented at 7:35 PM on April 28, 2017: member

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 commented at 7:53 PM on April 28, 2017: contributor

@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 commented at 8:11 PM on April 28, 2017: member

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

[test] Add test for getmemoryinfo

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.

d4668f35ab

jimmysong force-pushed on May 9, 2017

jimmysong commented at 9:49 PM on May 9, 2017: contributor

Rebased and ready. Thanks @MarcoFalke

paveljanik commented at 2:50 PM on May 15, 2017: contributor

reACK https://github.com/bitcoin/bitcoin/pull/10257/commits/d4668f35ab230083e39ab05857ccae4fe77dcb50

laanwj merged this on May 17, 2017

laanwj closed this on May 17, 2017

laanwj referenced this in commit ea1fd43bb9 on May 17, 2017

attilaaf referenced this in commit 9184637847 on May 25, 2019

PastaPastaPasta referenced this in commit ce8d2a1e93 on Jun 10, 2019

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

DrahtBot locked this on Sep 8, 2021