rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs

brakmic commented at 11:49 pm on April 8, 2020: contributor

This PR expands the JSONs returned from getbalances, gettransaction and getwalletinfo RPCs by adding a new property: lastprocessedblock that contains the hash and height of the block.

getbalances

 0./src/bitcoin-cli -regtest getbalances
 1{
 2  "mine": {
 3    "trusted": 14284.37500000,
 4    "untrusted_pending": 0.00000000,
 5    "immature": 254.68750000
 6  },
 7  "lastprocessedblock": {
 8   "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
 9   "height": 786
10  }
11}

gettransaction

0./src/bitcoin-cli -regtest gettransaction fa3fd022eaccd003d93a02f31848fc34d81e4e07a9a2e7690e49f92c8c1004cb
1 [...snip...]
2  "lastprocessedblock": {
3    "hash": "592d87f3f7ff48ddf1ae07dcd0003de9e484c0df00be3b78a9681c84d607e030",
4    "height": 796
5  }
6}

getwalletinfo

 0./src/bitcoin-cli -regtest getwalletinfo
 1{
 2  "walletname": "",
 3  "walletversion": 169900,
 4  "balance": 14315.62500000,
 5  [...snip...]
 6  "lastprocessedblock": {
 7       "hash": "592d87f3f7ff48ddf1ae07dcd0003de9e484c0df00be3b78a9681c84d607e030",
 8       "height": 796
 9    }
10}

Changes:

Introduced a new wallet function GetLastBlockHash to return m_last_block_processed
Introduced helper function AppendLastProcessedBlock to insert JSON objects
Introduced static RPCResult variable RESULT_LAST_PROCESSED_BLOCK for JSON reuse
Added tests in tests/functional/wallet_balance.py
Added release-notes-18570.md

The motivation for this PR can be found here #18567

The idea to make lastprocessedblock an object that contains both hash and height is from vasild. Originally, only the hash was shown.

Fixes https://github.com/bitcoin/bitcoin/issues/18567

fanquake added the label RPC/REST/ZMQ on Apr 8, 2020

in src/wallet/rpcwallet.cpp:2405 in 090181649d outdated

2399@@ -2399,6 +2400,11 @@ static UniValue getbalances(const JSONRPCRequest& request)
2400     const auto bal = wallet.GetBalance();
2401     UniValue balances{UniValue::VOBJ};
2402     {
2403+        Optional<int> height = locked_chain->getHeight();
2404+        if (height) {
2405+            uint256 lastblock = locked_chain->getBlockHash(height.get());

MarcoFalke commented at 0:05 am on April 9, 2020:

this is incorrect, I believe. The chain can proceed to the next block after the call to BlockUntilSyncedToCurrentChain, but the wallet may not catch up until the lock is taken in the next line.

This should be wallet.m_last_block_processed

brakmic commented at 9:30 am on April 9, 2020:

Thanks!

Have changed the code by introducing a new wallet function GetLastBlockHash() that returns m_last_block_processed .

MarcoFalke commented at 0:06 am on April 9, 2020: member

Concept ACK. Could:

Add tests
Add documentation about this guideline to doc/developer-notes.md where BlockUntilSyncedToCurrentChain is also documented

in src/wallet/rpcwallet.cpp:2372 in 090181649d outdated

2368@@ -2369,6 +2369,7 @@ static UniValue getbalances(const JSONRPCRequest& request)
2369         RPCResult{
2370             RPCResult::Type::OBJ, "", "",
2371             {
2372+                {RPCResult::Type::STR_HEX, "lastblock", "The hash of the block the balances were valid on"},

MarcoFalke commented at 0:18 am on April 9, 2020:

Could move this last, since it is the least-important information?

brakmic commented at 9:30 am on April 9, 2020:

Done.

brakmic commented at 9:32 am on April 9, 2020: contributor

Concept ACK. Could:

Add tests

Add documentation about this guideline to doc/developer-notes.md where BlockUntilSyncedToCurrentChain is also documented

Many thanks.

Have added a new functional test rpc_getbalances.py and a new entry on getbalances and that it uses m_last_block_processed to populate lastblockfield.

However, not sure if the docs I added is sufficient enough.

brakmic closed this on Apr 9, 2020

brakmic commented at 9:37 am on April 9, 2020: contributor

And again, GitHub closed my pull request. This makes no sense, I just force pushed it. Ok, will have to open a new PR with the same content. Sorry!

brakmic commented at 9:37 am on April 9, 2020: contributor

@fanquake Maybe you can reopen it? We had this problem in the past already. Not sure why GitHub is doing it.

fanquake commented at 9:38 am on April 9, 2020: member

I cannot. You’ll likely have to open a new PR.

brakmic commented at 9:39 am on April 9, 2020: contributor

I cannot. You’ll likely have to open a new PR.

Ok, no problem.

promag commented at 9:44 am on April 9, 2020: member

@brakmic see this:

brakmic commented at 9:45 am on April 9, 2020: contributor

@promag Thanks. I’ll try it.

brakmic commented at 9:51 am on April 9, 2020: contributor

@brakmic see this:

Reopened. Thanks, @promag

brakmic reopened this on Apr 9, 2020

vasild commented at 10:10 am on April 9, 2020: member

Concept ACK.

Very good idea, to add “valid as of …” stamp to the returned data. What about also adding the height to make it easy/obvious which one is newer, assuming a few requests:

0"lastblock": {
1    "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
2    "height": 12345
3}

But it’s good even without that.

Thanks!

in doc/developer-notes.md:1083 in eaf2888329 outdated

1079@@ -1080,6 +1080,10 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1080     until the wallet is caught up to the chainstate as of the RPC call's entry.
1081     This also makes the API much easier for RPC clients to reason about.
1082 
1083+- The RPC call `getbalances` also returns `m_last_block_processed` in the `lastblock`

promag commented at 10:12 am on April 9, 2020:

s/m_last_block_processed/lastblock/

brakmic commented at 10:22 am on April 9, 2020:

I don’t understand, sorry. We are returning m_last_block_processed in the field lastblock.

promag commented at 10:38 am on April 9, 2020:

Sorry, I mean that we could avoid C++ variable names here. I was thinking something like

0- The `getbalances` RPC now returns `lastblock` which is the wallet's last processed block
1  hash at the time the balances were calculated.

brakmic commented at 11:00 am on April 9, 2020:

Thanks. Have update the docs.

brakmic commented at 10:14 am on April 9, 2020: contributor

Concept ACK.

Very good idea, to add “valid as of …” stamp to the returned data. What about also adding the height to make it easy/obvious which one is newer, assuming a few requests:
0"lastblock": {
1    "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
2    "height": 12345
3}
But it’s good even without that.

Thanks!

As wallet already has GetLastBlockHeight()we could make lastblock a UniValue::VOBJ and populate it with hash and height.

in doc/developer-notes.md:1085 in eaf2888329 outdated

1079@@ -1080,6 +1080,10 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1080     until the wallet is caught up to the chainstate as of the RPC call's entry.
1081     This also makes the API much easier for RPC clients to reason about.
1082 
1083+- The RPC call `getbalances` also returns `m_last_block_processed` in the `lastblock`
1084+  field. As this call locks the chainstate we can also inform about the block
1085+  the balances were valid on.

promag commented at 10:15 am on April 9, 2020:

Maybe worth mentioning the result shouldn’t be cached because importing new keys could invalidate it. Instead of “were valid on” maybe say “were calculated/computed/evaluated on”?

brakmic commented at 10:27 am on April 9, 2020:

Thanks. Have updated the wording and added a warning regarding caching of results.

in src/wallet/rpcwallet.cpp:2385 in dc862b6fda outdated

2381@@ -2382,6 +2382,7 @@ static UniValue getbalances(const JSONRPCRequest& request)
2382                     {RPCResult::Type::STR_AMOUNT, "untrusted_pending", "untrusted pending balance (outputs created by others that are in the mempool)"},
2383                     {RPCResult::Type::STR_AMOUNT, "immature", "balance from immature coinbase outputs"},
2384                 }},
2385+                {RPCResult::Type::STR_HEX, "lastblock", "The hash of the block the balances were valid on"},

promag commented at 10:33 am on April 9, 2020:

calculated?

brakmic commented at 10:46 am on April 9, 2020:

Yes, this too was changed when I converted lastblock into an object that contains both hash and height.

in test/functional/rpc_getbalances.py:32 in dc862b6fda outdated

27+        balances = self.nodes[0].getbalances()
28+        assert_greater_than(balances['mine']['immature'], 0)
29+        assert_equal(balances['mine']['trusted'], 0)
30+        # Check if lastblock present and of type hash
31+        lastblock1 = balances['lastblock']
32+        assert_is_hash_string(lastblock1)

promag commented at 10:40 am on April 9, 2020:

Compare with self.nodes[0].getbestblockhash() instead?

brakmic commented at 10:47 am on April 9, 2020:

Ok, will update the tests.

brakmic commented at 10:49 am on April 9, 2020: contributor

Concept ACK.

Very good idea, to add “valid as of …” stamp to the returned data. What about also adding the height to make it easy/obvious which one is newer, assuming a few requests:
0"lastblock": {
1    "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
2    "height": 12345
3}
But it’s good even without that.

Thanks!

Have updated the code, please check.

in test/functional/rpc_getbalances.py:35 in 0246a6c89f outdated

30+        # Keep current best block hash & height
31+        prev_hash = self.nodes[0].getbestblockhash()
32+        prev_height = self.nodes[0].getblock(prev_hash)['height']
33+        # Move the chain again
34+        self.nodes[0].generatetoaddress(5, self.nodes[0].get_deterministic_priv_key().address)
35+        # TEST: lastblock should be a valid hash

promag commented at 11:05 am on April 9, 2020:

IMO these comments are unnecessary.

brakmic commented at 12:14 pm on April 9, 2020:

Ok, will update (already updated) tests :)

in test/functional/test_runner.py:200 in 0246a6c89f outdated

196@@ -197,6 +197,7 @@
197     'feature_minchainwork.py',
198     'rpc_estimatefee.py',
199     'rpc_getblockstats.py',
200+    'rpc_getbalances.py',

jonatack commented at 11:11 am on April 9, 2020:

Note that test/functional/wallet_balance.py already exists and these tests could be placed there, particularly as any of the tests in that file that don’t call getbalances yet should be converted to it (see #18451 which does that).

brakmic commented at 12:13 pm on April 9, 2020:

Thanks! Have updated the test and removed the now redundant rpc_getbalance.

in test/functional/rpc_getbalances.py:34 in 0246a6c89f outdated

29+        assert_equal(balances['mine']['trusted'], 0)
30+        # Keep current best block hash & height
31+        prev_hash = self.nodes[0].getbestblockhash()
32+        prev_height = self.nodes[0].getblock(prev_hash)['height']
33+        # Move the chain again
34+        self.nodes[0].generatetoaddress(5, self.nodes[0].get_deterministic_priv_key().address)

promag commented at 11:12 am on April 9, 2020:

IMO this 2nd round is unnecessary. I think you could factor out a test_getbalances function that would call getbalances and assert the result (and maybe just use getblockchaininfo to get chain tip). Then here you would do

0test_getbalances(self.nodes[0], ...)
1generatetoaddress(...)
2test_getbalances(self.nodes[0], ...)

brakmic commented at 12:16 pm on April 9, 2020:

Meanwhile, I’ve moved the tests into wallet_balance. There I’m just calling a single function to run all those tests: test_lastblock_json_field

Please, check.

promag commented at 11:12 am on April 9, 2020: member

Agree with @jonatack.

in doc/developer-notes.md:1084 in 0246a6c89f outdated

1079@@ -1080,6 +1080,10 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1080     until the wallet is caught up to the chainstate as of the RPC call's entry.
1081     This also makes the API much easier for RPC clients to reason about.
1082 
1083+- The `getbalances` RPC now returns `lastblock` which contains the wallet's last processed block
1084+  hash and height at the time the balances were calculated. As this call locks the chainstate we can also inform about the block

jonatack commented at 11:13 am on April 9, 2020:

“we can also inform about the balances” -> I don’t quite understand what this means.

brakmic commented at 11:25 am on April 9, 2020:

I was pointing to this sentence from the original issue:

Those calls could benefit if the block hash they were valid on was included in the response. For example, immature balance can only change when the chainstate changes.

Maybe I should reformulate the sentence.

brakmic commented at 12:40 pm on April 9, 2020:

Have now removed the problematic sentence.

in doc/developer-notes.md:1083 in 0246a6c89f outdated

1079@@ -1080,6 +1080,10 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1080     until the wallet is caught up to the chainstate as of the RPC call's entry.
1081     This also makes the API much easier for RPC clients to reason about.
1082 
1083+- The `getbalances` RPC now returns `lastblock` which contains the wallet's last processed block

jonatack commented at 11:13 am on April 9, 2020:

This seems to be information for the release note?

promag commented at 11:17 am on April 9, 2020:

🤣 indeed! my brain read release-notes.md..

brakmic commented at 11:41 am on April 9, 2020:

Should I change anything? Add further info?

promag commented at 11:57 am on April 9, 2020:

Move this hunk to release notes, not developer notes.

brakmic commented at 11:59 am on April 9, 2020:

Ok, but what about the rest of the paragraph in developer-notes? Maybe I should rather keep it there as well?

brakmic commented at 12:23 pm on April 9, 2020:

@promag It seems, I can only change the 0.20.0 release (draft) notes here: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/0.20.0-Release-Notes-Draft

The release-notes.md I have is pointing at it.

I’m not sure if I should put an entry in 0.20.0 release notes when this PR is still unmerged.

jonatack commented at 12:50 pm on April 9, 2020:

@brakmic I don’t know if this is for 0.20, as the feature freeze was 3 weeks ago, but see the release notes section in developer-notes.md. Normally you don’t edit release-notes.md, rather instead you create a file in doc/release-notes/ called release-note-18570.md.

brakmic commented at 1:14 pm on April 9, 2020:

Added respective doc.

brakmic renamed this:
~~rpc: return block hash in getbalances json~~
rpc: return block hash and height in getbalances json
on Apr 9, 2020

jonasschnelli commented at 12:22 pm on April 9, 2020: contributor

Code review ACK. Needs git cleanup (squashing). Verified that there are no concurrency issues between getting the balance and the fetching the wallets bestblock.

nit: i’m not 100% sure about the term lastblock. Maybe we should use lastprocessedblock.

brakmic commented at 12:25 pm on April 9, 2020: contributor

Code review ACK. Needs git cleanup (squashing). Verified that there are no concurrency issues between getting the balance and the fetching the wallets bestblock.

nit: i’m not 100% sure about the term lastblock. Maybe we should use lastprocessedblock.

Thanks. Regarding lastblock: yes, it makes more sense. Will update the variable name throughout the code & docs.

in doc/developer-notes.md:1083 in 654d7812e5 outdated

1079@@ -1080,6 +1080,9 @@ A few guidelines for introducing and reviewing new RPC interfaces:
1080     until the wallet is caught up to the chainstate as of the RPC call's entry.
1081     This also makes the API much easier for RPC clients to reason about.
1082 
1083+- The `getbalances` RPC now returns `lastprocessedblock` which contains the wallet's last processed block

jonatack commented at 12:52 pm on April 9, 2020:

returns a lastprocessedblock JSON object

MarcoFalke added this to the milestone 0.21.0 on Apr 9, 2020

in test/functional/wallet_balance.py:14 in 654d7812e5 outdated

10@@ -11,6 +11,8 @@
11 from test_framework.util import (
12     assert_equal,
13     assert_raises_rpc_error,
14+    assert_greater_than,
15+    assert_is_hash_string,

jonatack commented at 12:53 pm on April 9, 2020:

nit: sort order

in test/functional/wallet_balance.py:269 in 654d7812e5 outdated

258@@ -257,5 +259,22 @@ def test_balances(*, fee_node_1=0):
259         assert_equal(self.nodes[0].getbalance(minconf=0), total_amount + 1)  # The reorg recovered our fee of 1 coin
260 
261 
262+        def test_lastprocessedblock_json_field():
263+            """
264+            Tests the lastprocessedblock-object by comparing
265+            the hashes & heights between generated blocks.
266+            """
267+            self.nodes[0].generatetoaddress(2, self.nodes[0].get_deterministic_priv_key().address)

jonatack commented at 12:54 pm on April 9, 2020:

Please add test logging if you retouch this, e.g. self.log.info("Test getbalances returns expected lastprocessedblock json object")

brakmic commented at 12:56 pm on April 9, 2020:

self.log.info is enough?

jonatack commented at 1:00 pm on April 9, 2020:

(I edited my comment just above with a suggestion)

in test/functional/wallet_balance.py:290 in d56f3a9558 outdated

273+            self.nodes[0].generatetoaddress(5, self.nodes[0].get_deterministic_priv_key().address)
274+            lastblock = self.nodes[0].getbalances()['lastprocessedblock']
275+            assert_is_hash_string(lastblock['hash'])
276+            assert_equal((prev_hash == lastblock['hash']), False)
277+            assert_greater_than(lastblock['height'], prev_height)
278+        test_lastprocessedblock_json_field()

theStack commented at 4:44 pm on April 10, 2020:

nit: would add an empty line in-between here, to mark the end of the function visually (same as for test_balances() above).

in doc/release-notes-18570.md:1 in d56f3a9558 outdated

0@@ -0,0 +1,2 @@
1+- The `getbalances` RPC now returns `lastprocessedblock` JSON object which contains the wallet's last processed block

theStack commented at 4:48 pm on April 10, 2020:

nit: would add the undefined article “a” in front of lastprocessedblock here

0- The `getbalances` RPC now returns a `lastprocessedblock` JSON object which contains the wallet's last processed block

brakmic commented at 5:34 pm on April 10, 2020:

Thanks!

theStack commented at 4:49 pm on April 10, 2020: member

ACK https://github.com/bitcoin/bitcoin/pull/18570/commits/d56f3a9558f23ac041f2ff111634b4ac827e32e1 (modulo two nits)

theStack commented at 5:52 pm on April 10, 2020: member

@brakmic: Don’t know how strict the rules are with those kinds of nits, but the local diff shows some trailing whitespace: grafik

brakmic commented at 5:54 pm on April 10, 2020: contributor

@brakmic: Don’t know how strict the rules are with those kinds of nits, but the local diff shows some trailing whitespace:

Ok, will wait for travis.

MarcoFalke commented at 6:00 pm on April 10, 2020: member

Travis fails as expected

MarcoFalke commented at 7:16 pm on April 10, 2020: member

gettransaction and getwalletinfo should also be trivial to amend, since they return an object already

brakmic commented at 7:19 pm on April 10, 2020: contributor

gettransaction and getwalletinfo should also be trivial to amend, since they return an object already

Ok. Should I try to include them in this PR as well?

MarcoFalke commented at 7:20 pm on April 10, 2020: member

Ok. Should I try to include them in this PR as well?

Yes, from what I could tell, adding it there seemed to be uncontroversial.

brakmic renamed this:
~~rpc: return block hash and height in getbalances json~~
rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs
on Apr 10, 2020

brakmic commented at 8:18 pm on April 10, 2020: contributor

Ok. Should I try to include them in this PR as well?

Yes, from what I could tell, adding it there seemed to be uncontroversial.

Updated PR info, expanded code, tests & release notes.

in src/wallet/rpcwallet.cpp:1702 in 7c1173049c outdated

1694@@ -1695,6 +1695,11 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1695                         {
1696                             {RPCResult::Type::ELISION, "", "Equivalent to the RPC decoderawtransaction method, or the RPC getrawtransaction method when `verbose` is passed."},
1697                         }},
1698+                        {RPCResult::Type::OBJ, "lastprocessedblock", "hash and height of the block this transaction information was generated on",
1699+                            {
1700+                            {RPCResult::Type::STR_HEX, "hash", "hash of the block this transaction information was generated on"},
1701+                            {RPCResult::Type::NUM, "height", "height of the block this transaction information was generated on"},
1702+                        }},

MarcoFalke commented at 9:09 pm on April 10, 2020:

Could this be made a static const RPCResult RESULT_LAST_PROCESSED_BLOCK{RPCResult::Type::OBJ, "lastprocessedbl... to avoid boilerplate?

0                        RESULT_LAST_PROCESSED_BLOCK,

in src/wallet/rpcwallet.cpp:1764 in 7c1173049c outdated

1757@@ -1753,6 +1758,11 @@ static UniValue gettransaction(const JSONRPCRequest& request)
1758         entry.pushKV("decoded", decoded);
1759     }
1760 
1761+    UniValue lastprocessedblock{UniValue::VOBJ};
1762+    lastprocessedblock.pushKV("hash", pwallet->GetLastBlockHash().GetHex());
1763+    lastprocessedblock.pushKV("height", pwallet->GetLastBlockHeight());
1764+    entry.pushKV("lastprocessedblock", lastprocessedblock);

MarcoFalke commented at 9:10 pm on April 10, 2020:

Could this be made a helper to avoid boilerplate?

0    AppendLastProccedBlock(entry);

in test/functional/wallet_balance.py:283 in 7c1173049c outdated

278+            assert_greater_than(lastblock['height'], prev_height)
279+            self.log.info("Test getwalletinfo returns expected lastprocessedblock json object")
280+            walletinfo = self.nodes[0].getwalletinfo()
281+            assert_greater_than(walletinfo['lastprocessedblock']['height'], 0)
282+            assert_is_hash_string(walletinfo['lastprocessedblock']['hash'])
283+            self.log.info("Test gettransaction returns expected lastprocessedblock json object")

MarcoFalke commented at 9:12 pm on April 10, 2020:

nit: Could add newlines before every log statement to aid readability?

MarcoFalke approved

MarcoFalke commented at 9:13 pm on April 10, 2020: member

ACK looks good, just nits

in test/functional/wallet_balance.py:264 in 7c1173049c outdated

258@@ -257,5 +259,35 @@ def test_balances(*, fee_node_1=0):
259         assert_equal(self.nodes[0].getbalance(minconf=0), total_amount + 1)  # The reorg recovered our fee of 1 coin
260 
261 
262+        def test_lastprocessedblock_json_field():
263+            """
264+            Tests the lastprocessedblock JSON object in getbalances, getwalletinfo

MarcoFalke commented at 9:14 pm on April 10, 2020:

0            Tests the lastprocessedblock JSON object in getbalances, getwalletinfo

in test/functional/wallet_balance.py:262 in 7c1173049c outdated

258@@ -257,5 +259,35 @@ def test_balances(*, fee_node_1=0):
259         assert_equal(self.nodes[0].getbalance(minconf=0), total_amount + 1)  # The reorg recovered our fee of 1 coin
260 
261 
262+        def test_lastprocessedblock_json_field():

MarcoFalke commented at 9:15 pm on April 10, 2020:

nit: no need to make this a function, because it is only called once. Could inline?

brakmic commented at 10:04 pm on April 10, 2020: contributor

ACK looks good, just nits

Thanks.

Have added:

static RPCResult variable RESULT_LAST_PROCESSED_BLOCK
lock-aware AppendLastProcessedBlock function
fixed styling + removed redundant function call in wallet_balance.py

in src/wallet/rpcwallet.cpp:59 in 1914dad299 outdated

50@@ -51,6 +51,19 @@ static inline bool GetAvoidReuseFlag(const CWallet* const pwallet, const UniValu
51     return avoid_reuse;
52 }
53 
54+static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "lastprocessedblock", "hash and height of the block this information was generated on",{
55+    {RPCResult::Type::STR_HEX, "hash", "hash of the block this information was generated on"},
56+    {RPCResult::Type::NUM, "height", "height of the block this information was generated on"}}
57+};
58+
59+void AppendLastProcessedBlock(UniValue& entry, const CWallet* const pwallet) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

MarcoFalke commented at 10:13 pm on April 10, 2020:

nit, to make sure this is not nullptr

0void AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)

brakmic commented at 10:17 pm on April 10, 2020:

Yeah, in getbalances they use a reference, but the other two functions use pwallet. My first thought was: let’s change this to a reference, but this would be an unrelated task.

Who knows, maybe later, in a new PR: “transition from pwallet to wallet&”…if possible.

MarcoFalke approved

MarcoFalke commented at 10:13 pm on April 10, 2020: member

re-ACK

DrahtBot commented at 4:19 am on April 11, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18788 (wallet: tests: Update more tests to work with descriptor wallets by achow101)
#18354 (Protect wallet by using shared pointers by bvbfan)

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.

theStack commented at 12:42 pm on April 11, 2020: member

c99444c4f127d9d33c26410206b5ed536e613c14 fails with the functional test wallet_basic.py on my machine:

 0...
 12020-04-11T12:38:15.577000Z TestFramework (INFO): Testing gettransaction response with different arguments...
 22020-04-11T12:38:15.584000Z TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/test_framework.py", line 112, in main
 5    self.run_test()
 6  File "test/functional/wallet_basic.py", line 520, in run_test
 7    assert_equal(set([*tx]), expected_fields)
 8  File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/util.py", line 46, in assert_equal
 9    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
10AssertionError: not({'time', 'hex', 'walletconflicts', 'timereceived', 'bip125-replaceable', 'confirmations', 'txid', 'details', 'amount', 'trusted', 'lastprocessedblock', 'fee'} == frozenset({'time', 'hex', 'walletconflicts', 'timereceived', 'bip125-replaceable', 'confirmations', 'txid', 'trusted', 'details', 'amount', 'fee'}))

brakmic commented at 12:46 pm on April 11, 2020: contributor

c99444c fails with the functional test wallet_basic.py on my machine:

On my machine it looks OK:

 0./test/functional/test_runner.py wallet_basic                                           (4s 289ms)  
 1Temporary test directory at /var/folders/7q/4ffytzk562dd2ky4bfg9_w7h0000gn/T/test_runner_₿_🏃_20200411_144508
 2Remaining jobs: [wallet_basic.py]
 31/1 - wallet_basic.py passed, Duration: 51 s                                                          
 4
 5TEST            | STATUS    | DURATION
 6
 7wallet_basic.py | ✓ Passed  | 51 s
 8
 9ALL             | ✓ Passed  | 51 s (accumulated) 
10Runtime: 51 s

Do you have the latest version? I just changed a few things, like rpcdump.cpp.

–EDIT … sorry, wrong PR. I am also working on another one. Will check again!

MarcoFalke commented at 12:53 pm on April 11, 2020: member

It fails on travis and appveyor as well

brakmic commented at 1:11 pm on April 11, 2020: contributor

It fails on travis and appveyor as well

Will check it again.

brakmic commented at 1:22 pm on April 11, 2020: contributor

Interesting, on my machine (a macOS Catalina 10.15.4) I am still not getting any errors when executing ./test/functional/test_runner.py wallet_basic.

Will have to dig deeper, it seems.

brakmic commented at 1:25 pm on April 11, 2020: contributor

@MarcoFalke travis isn’t showing any error logs? Only that it wasn’t able to fetch them.

MarcoFalke commented at 1:32 pm on April 11, 2020: member

Error is the same on appveyor: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/32103450#L4902

Make sure to run git log -1 && git status && make clean && make && ./test/functional/wallet_basic.py to reproduce the issue.

brakmic commented at 1:58 pm on April 11, 2020: contributor

Error is the same on appveyor: https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/32103450#L4902

Make sure to run git log -1 && git status && make clean && make && ./test/functional/wallet_basic.py to reproduce the issue.

The frozenset expected_fields on line 513 needed lastprocessedblock to pass the test.

in test/functional/wallet_basic.py:515 in 0bfce2c16b outdated

510@@ -511,7 +511,8 @@ def run_test(self):
511                                  "category": baz["category"],
512                                  "vout":     baz["vout"]}
513         expected_fields = frozenset({'amount', 'bip125-replaceable', 'confirmations', 'details', 'fee',
514-                                     'hex', 'time', 'timereceived', 'trusted', 'txid', 'walletconflicts'})
515+                                     'hex', 'time', 'timereceived', 'trusted', 'txid', 'walletconflicts',
516+                                     'lastprocessedblock'})

jonatack commented at 2:22 pm on April 11, 2020:

nit: sort

brakmic commented at 2:35 pm on April 11, 2020:

Thanks. Corrected.

brakmic commented at 5:41 pm on April 11, 2020: contributor

@MarcoFalke

The interface_rest test fails on appveyor with OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted

MarcoFalke commented at 6:44 pm on April 11, 2020: member

ACK 0ac3e82d210635b9e5cb72be481ea699902920d8

in test/functional/wallet_balance.py:287 in 0ac3e82d21 outdated

282+
283+        self.log.info("Test gettransaction returns expected lastprocessedblock json object")
284+        txid = self.nodes[0].sendtoaddress(self.nodes[0].getnewaddress(), 1)
285+        self.nodes[0].generatetoaddress(6, self.nodes[0].get_deterministic_priv_key().address)
286+        tx_info = self.nodes[0].gettransaction(txid)
287+        assert_greater_than(walletinfo['lastprocessedblock']['height'], 0)

theStack commented at 8:03 pm on April 11, 2020:

I think you want to access the just one line above returned tx_info here, not walletinfo?

brakmic commented at 8:06 pm on April 11, 2020:

Thanks! Yes, walletinfo in this part is redundant as it got already checked in the previous code block.

in src/wallet/rpcwallet.cpp:67 in 81955a0dc1 outdated

59+void AppendLastProcessedBlock(UniValue& entry, const CWallet& wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
60+{
61+    AssertLockHeld(wallet.cs_wallet);
62+    UniValue lastprocessedblock{UniValue::VOBJ};
63+    lastprocessedblock.pushKV("hash", wallet.GetLastBlockHash().GetHex());
64+    lastprocessedblock.pushKV("height", wallet.GetLastBlockHeight());

vasild commented at 1:50 pm on April 13, 2020:

GetLastBlockHeight() returns m_last_block_processed_height and there is this strange scary comment for it:

Height is a pointer on node’s tip and doesn’t imply that the wallet has scanned sequentially all blocks up to this one.

Introduced in https://github.com/bitcoin/bitcoin/pull/15931/files#diff-12635a58447c65585f51d32b7e04075bR697-R698

Before that it was

Note that this is not how far we’ve processed…

from https://github.com/bitcoin/bitcoin/pull/10286/files#diff-12635a58447c65585f51d32b7e04075bR732-R734

Does this contradict with what we are trying to do here?

MarcoFalke commented at 2:22 pm on April 13, 2020:

Does this contradict with what we are trying to do here?

Yes it does. Not sure how we should deal with old wallets that are catching up with the chain after being just loaded.

vasild commented at 2:11 pm on April 13, 2020: member

Looks good, except the below and appveyor failure, maybe unrelated:

OSError: [WinError 10048] Only one usage of each socket address (protocol/network address/port) is normally permitted

luke-jr commented at 4:37 am on April 23, 2020: member

I’m not sure this is the right approach, interface-wise, especially for calls like gettransaction, where the info gets embedded in the JSON transaction itself.

Maybe it’d be better to just guarantee atomicity for batch requests?

vasild commented at 7:21 am on April 23, 2020: member

Maybe it’d be better to just guarantee atomicity for batch requests?

Atomicity of batch requests would be good from user point of view, no doubt about it. But it would not fully cover what this PR is trying to do - provide a kind of “stamp” for the returned data - “this is valid as of block XYZ (height 123)”. I think this is important given the volatility of the data - it may no longer be accurate when it gets to the client or the client may do something with it at a later time.

luke-jr commented at 12:37 pm on April 23, 2020: member

You would batch any call with getstamp

MarcoFalke commented at 2:14 pm on April 23, 2020: member

You would batch any call with getstamp

So wallets are unusable by clients that don’t have batch support implemented?

luke-jr commented at 4:55 pm on April 23, 2020: member

“Unusable” seems a bit strong here.

But yes, “I don’t want to implement it” isn’t an excuse to pick a bad interface…

DrahtBot added the label Needs rebase on Apr 27, 2020

DrahtBot commented at 0:34 am on April 27, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

brakmic closed this on Apr 27, 2020

brakmic reopened this on Apr 27, 2020

fanquake removed the label Needs rebase on Apr 27, 2020

rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs 1e868bbbb1

brakmic closed this on May 10, 2020

luke-jr referenced this in commit 44ec7f1ff8 on Jun 9, 2020

MarcoFalke added the label Up for grabs on Jun 21, 2020

DrahtBot locked this on Feb 15, 2022

fanquake removed the label Up for grabs on Sep 14, 2022

rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs #18570

Conflicts