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: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs #18570

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

brakmic
Copy link
Contributor

@brakmic brakmic commented Apr 8, 2020

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

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Concept ACK. Could:

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

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
@brakmic
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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 brakmic closed this Apr 9, 2020
@brakmic
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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

@fanquake
Copy link
Member

fanquake commented Apr 9, 2020

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

@brakmic
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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

Ok, no problem.

@promag
Copy link
Member

promag commented Apr 9, 2020

@brakmic see this:

Screenshot 2020-04-09 at 10 43 55

@brakmic
Copy link
Contributor Author

brakmic commented Apr 9, 2020

@promag
Thanks.
I'll try it.

@brakmic
Copy link
Contributor Author

brakmic commented Apr 9, 2020

@brakmic see this:

Screenshot 2020-04-09 at 10 43 55

Reopened. Thanks, @promag

@brakmic brakmic reopened this Apr 9, 2020
@vasild
Copy link
Contributor

vasild commented Apr 9, 2020

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
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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 Outdated Show resolved Hide resolved
doc/developer-notes.md Outdated Show resolved Hide resolved
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
@brakmic
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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 Outdated Show resolved Hide resolved
Copy link
Member

@promag promag left a comment

Agree with @jonatack.

test/functional/rpc_getbalances.py Outdated Show resolved Hide resolved
test/functional/rpc_getbalances.py Outdated Show resolved Hide resolved
doc/developer-notes.md Outdated Show resolved Hide resolved
doc/developer-notes.md Outdated Show resolved Hide resolved
@brakmic brakmic changed the title rpc: return block hash in getbalances json rpc: return block hash and height in getbalances json Apr 9, 2020
@jonasschnelli
Copy link
Contributor

jonasschnelli commented Apr 9, 2020

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
Copy link
Contributor Author

brakmic commented Apr 9, 2020

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 Outdated Show resolved Hide resolved
@MarcoFalke MarcoFalke added this to the 0.21.0 milestone Apr 9, 2020
test/functional/wallet_balance.py Outdated Show resolved Hide resolved
test/functional/wallet_balance.py Outdated Show resolved Hide resolved
@brakmic
Copy link
Contributor Author

brakmic commented Apr 11, 2020

It fails on travis and appveyor as well

Will check it again.

@brakmic
Copy link
Contributor Author

brakmic commented Apr 11, 2020

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
Copy link
Contributor Author

brakmic commented Apr 11, 2020

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

@MarcoFalke
Copy link
Member

MarcoFalke commented Apr 11, 2020

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
Copy link
Contributor Author

brakmic commented Apr 11, 2020

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 Outdated Show resolved Hide resolved
@brakmic
Copy link
Contributor Author

brakmic commented Apr 11, 2020

@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
Copy link
Member

MarcoFalke commented Apr 11, 2020

ACK 0ac3e82

test/functional/wallet_balance.py Outdated Show resolved Hide resolved
Copy link
Contributor

@vasild vasild left a comment

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

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

@vasild vasild Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@MarcoFalke MarcoFalke Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

luke-jr commented Apr 23, 2020

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
Copy link
Contributor

vasild commented Apr 23, 2020

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
Copy link
Member

luke-jr commented Apr 23, 2020

You would batch any call with getstamp

@MarcoFalke
Copy link
Member

MarcoFalke commented Apr 23, 2020

You would batch any call with getstamp

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

@luke-jr
Copy link
Member

luke-jr commented Apr 23, 2020

"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
Copy link
Contributor

DrahtBot commented Apr 27, 2020

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

@brakmic brakmic closed this May 10, 2020
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this issue Jun 9, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants