8000 rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo by aureleoules · Pull Request #26094 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo #26094

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 2, 2023
Merged

rpc: Return block hash & height in getbalances, gettransaction and getwalletinfo #26094

merged 1 commit into from
May 2, 2023

Conversation

aureleoules
Copy link
Contributor

Reopens #18570 and closes #18567.
I have rebased the original PR.
Not sure why the original got closed as it was about to get merged.

@aureleoules aureleoules force-pushed the return-blockhash-with-wallet-calls branch from 64fc793 to 7bf1c59 Compare September 14, 2022 17:21
@DrahtBot
Copy link
Contributor
DrahtBot commented Sep 14, 2022 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK achow101
Concept ACK jarolrod
Stale ACK willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
  • #25634 (wallet, tests: Expand and test when the blank wallet flag should be un/set by achow101)

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.

@DrahtBot DrahtBot mentioned this pull request Sep 15, 2022
Merged
@luke-jr
Copy link
Member
luke-jr commented Sep 20, 2022

As I said previously,

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?

This was referenced Sep 22, 2022
Draft
Closed
jarolrod
jarolrod reviewed Sep 23, 2022
View reviewed changes
Copy link
Member
@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

Concept ACK

This does what it says it does. I think it's fine to add these fields, I can imagine it would be useful. Tested and sanity checked the code, and ran tests locally.

@DrahtBot DrahtBot mentioned this pull request Sep 28, 2022
Merged
@aureleoules aureleoules force-pushed the return-blockhash-with-wallet-calls branch 3 times, most recently from b3752eb to 37a05b4 Compare October 3, 2022 17:01
willcl-ark
willcl-ark approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member
@willcl-ark willcl-ark left a comment 8000

Choose a reason for hiding this comment

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

ACK 37a05b4

Returns the blockheight and hash the aforementioned RPCs were run against. Feels like a win for users of the RPCs.

@DrahtBot DrahtBot mentioned this pull request Dec 15, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Feb 3, 2023
Closed
@achow101
Copy link
Member
achow101 commented Feb 3, 2023

ACK 37a05b4

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

I don't really see how that would be possible to implement in a way that wouldn't be detrimental or fragile. It seems like it would have to be implemented with an explicit list of RPCs that must be done atomically in order to avoid any RPC just locking things up. Such a list could fall out of date as new RPCs are added and people forget, so it would be kind of fragile.

@achow101
Copy link
Member

There's a test failure in wallet_orphanedreward.py.

256/263 - wallet_orphanedreward.py failed, Duration: 17 s

stdout:
2023-04-26T12:57:28.311000Z TestFramework (INFO): PRNG seed is: 5570008732445289618
2023-04-26T12:57:28.313000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20230426_085557/wallet_orphanedreward_19
2023-04-26T12:57:40.283000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/andy/bitcoin/bitcoin/master/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/home/andy/bitcoin/bitcoin/master/gcc_build/enable-debug/../../test/functional/wallet_orphanedreward.py", line 68, in run_test
    assert_equal(self.nodes[1].getbalances(), pre_reorg_conf_bals)
  File "/home/andy/bitcoin/bitcoin/master/test/functional/test_framework/util.py", line 56, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not({'mine': {'trusted': Decimal('35.00000000'), 'untrusted_pending': Decimal('0E-8'), 'immature': Decimal('0E-8')}, 'lastprocessedblock': {'hash': '20e9d23d7183d38eb6789704c3e658d6915178fd95d4479edfd5c3f2df023793', 'height': 305}} == {'mine': {'trusted': Decimal('35.00000000'), 'untrusted_pending': Decimal('0E-8'), 'immature': Decimal('0E-8')}, 'lastprocessedblock': {'hash': '5503776f6d7a7dc6f769176922582a6333a0d6821ec0c2dc30f9114f45fa2351', 'height': 302}})
2023-04-26T12:57:40.334000Z TestFramework (INFO): Stopping nodes
2023-04-26T12:57:40.436000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20230426_085557/wallet_orphanedreward_19
2023-04-26T12:57:40.436000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20230426_085557/wallet_orphanedreward_19/test_framework.log
2023-04-26T12:57:40.436000Z TestFramework (ERROR): 
2023-04-26T12:57:40.436000Z TestFramework (ERROR): Hint: Call /home/andy/bitcoin/bitcoin/master/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20230426_085557/wallet_orphanedreward_19' to consolidate all logs
2023-04-26T12:57:40.436000Z TestFramework (ERROR): 
2023-04-26T12:57:40.436000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2023-04-26T12:57:40.436000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2023-04-26T12:57:40.436000Z TestFramework (ERROR): 


stderr:

@brakmic @aureleoules
rpc: return block hash & height in getbalances, gettransaction & getw…
710b839 
…alletinfo JSONs

Co-authored-by: Aurèle Oulès <aurele@oules.com>
@aureleoules aureleoules force-pushed the return-blockhash-with-wallet-calls branch from 37a05b4 to 710b839 Compare April 26, 2023 14:08
@achow101
Copy link
Member
achow101 commented May 2, 2023

ACK 710b839

@DrahtBot DrahtBot requested a review from willcl-ark May 2, 2023 15:42
@achow101 achow101 merged commit da9f62f into bitcoin:master May 2, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 4, 2023
@achow101
Merge bitcoin#26094: rpc: Return block hash & height in getbalances, …
ab8a2a0 
…gettr
8000
ansaction and getwalletinfo

710b839 rpc: return block hash & height in getbalances, gettransaction & getwalletinfo JSONs (Harris)

Pull request description:

  Reopens bitcoin#18570 and closes bitcoin#18567.
  I have rebased the original PR.
  Not sure why the original got closed as it was about to get merged.

ACKs for top commit:
  achow101:
    ACK 710b839

Tree-SHA512: d4478d990be98b1642e9ffb2930987f4a224e8bd64e2e35a5dda927a54c509ec9d712cd0eac35dc2bb89f00a1678e530ce14d7445f1dd93aa3a4cce2bc9b130d
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Aug 16, 2023
@brakmic @luke-jr
rpc: return block hash & height in getbalances, gettransaction & getw…
63f63d9 
…alletinfo JSONs

Co-authored-by: Aurèle Oulès <aurele@oules.com>

Github-Pull: bitcoin#26094
Rebased-From: 710b839
@bitcoin bitcoin locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jarolrod jarolrod jarolrod left review comments

@willcl-ark willcl-ark Awaiting requested review from willcl-ark

Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return block hash with wallet calls
7 participants
@aureleoules @DrahtBot @luke-jr @achow101 @willcl-ark @jarolrod @brakmic
0