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

brakmic · 2020-04-08T23:49:52Z

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

./src/bitcoin-cli -regtest getbalances
{
  "mine": {
    "trusted": 14284.37500000,
    "untrusted_pending": 0.00000000,
    "immature": 254.68750000
  },
  "lastprocessedblock": {
   "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
   "height": 786
  }
}

gettransaction

./src/bitcoin-cli -regtest gettransaction fa3fd022eaccd003d93a02f31848fc34d81e4e07a9a2e7690e49f92c8c1004cb
 [...snip...]
  "lastprocessedblock": {
    "hash": "592d87f3f7ff48ddf1ae07dcd0003de9e484c0df00be3b78a9681c84d607e030",
    "height": 796
  }
}

getwalletinfo

./src/bitcoin-cli -regtest getwalletinfo
{
  "walletname": "",
  "walletversion": 169900,
  "balance": 14315.62500000,
  [...snip...]
  "lastprocessedblock": {
       "hash": "592d87f3f7ff48ddf1ae07dcd0003de9e484c0df00be3b78a9681c84d607e030",
       "height": 796
    }
}

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 #18567

maflcko

Concept ACK. Could:

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

src/wallet/rpcwallet.cpp

brakmic · 2020-04-09T09:32:42Z

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 · 2020-04-09T09:37:01Z

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 · 2020-04-09T09:37:32Z

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

fanquake · 2020-04-09T09:38:12Z

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

brakmic · 2020-04-09T09:39:14Z

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

Ok, no problem.

promag · 2020-04-09T09:44:29Z

@brakmic see this:

brakmic · 2020-04-09T09:45:56Z

@promag
Thanks.
I'll try it.

brakmic · 2020-04-09T09:51:48Z

@brakmic see this:

Reopened. Thanks, @promag

vasild · 2020-04-09T10:10:38Z

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:

"lastblock": {
    "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
    "height": 12345
}

But it's good even without that.

Thanks!

brakmic · 2020-04-09T10:14:46Z

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:
"lastblock": {
    "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
    "height": 12345
}
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.

doc/developer-notes.md

src/wallet/rpcwallet.cpp

doc/developer-notes.md

test/functional/rpc_getbalances.py

brakmic · 2020-04-09T10:49:43Z

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:
"lastblock": {
    "hash": "5ba7e8b9a9e6aed88b4641257ef13ecaace1211688f5b5fec99cad36e1650e5d",
    "height": 12345
}
But it's good even without that.

Thanks!

Have updated the code, please check.

test/functional/test_runner.py

promag

Agree with @jonatack.

test/functional/rpc_getbalances.py

doc/developer-notes.md

jonasschnelli · 2020-04-09T12:22:40Z

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 · 2020-04-09T12:25:06Z

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.

doc/developer-notes.md

test/functional/wallet_balance.py

brakmic · 2020-04-11T13:22:50Z

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 · 2020-04-11T13:25:29Z

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

maflcko · 2020-04-11T13:32:58Z

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 · 2020-04-11T13:58:54Z

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.

test/functional/wallet_basic.py

brakmic · 2020-04-11T17:41:50Z

@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

maflcko · 2020-04-11T18:44:44Z

ACK 0ac3e82

test/functional/wallet_balance.py

vasild

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

vasild · 2020-04-13T13:50:29Z

src/wallet/rpcwallet.cpp

+    AssertLockHeld(wallet.cs_wallet);
+    UniValue lastprocessedblock{UniValue::VOBJ};
+    lastprocessedblock.pushKV("hash", wallet.GetLastBlockHash().GetHex());
+    lastprocessedblock.pushKV("height", wallet.GetLastBlockHeight());


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?

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.

luke-jr · 2020-04-23T04:37:54Z

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 · 2020-04-23T07:21:29Z

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 · 2020-04-23T12:37:53Z

You would batch any call with getstamp

maflcko · 2020-04-23T14:14:43Z

You would batch any call with getstamp

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

luke-jr · 2020-04-23T16:55:50Z

"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 · 2020-04-27T00:34:57Z

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

…alletinfo JSONs

…alletinfo JSONs Github-Pull: bitcoin#18570 Rebased-From: 1e868bb

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

maflcko reviewed Apr 9, 2020

View reviewed changes

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

maflcko reviewed Apr 9, 2020

View reviewed changes

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

brakmic closed this Apr 9, 2020

brakmic reopened this Apr 9, 2020

promag reviewed Apr 9, 2020

View reviewed changes

doc/developer-notes.md Outdated Show resolved Hide resolved

doc/developer-notes.md Outdated Show resolved Hide resolved

promag reviewed Apr 9, 2020

View reviewed changes

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

doc/developer-notes.md Outdated Show resolved Hide resolved

test/functional/rpc_getbalances.py Outdated Show resolved Hide resolved