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: Improve scantxoutset response and help message #16285

Merged
merged 2 commits into from
Sep 9, 2019

Conversation

promag
Copy link
Member

@promag promag commented Jun 25, 2019

The new response keys height and bestblock allow the client to know at what point the scan took place.

The help message now has all the response keys (result and txouts were missing) and it's improved a bit. Note that searched_items key is renamed to txouts, considering scantxoutset is marked experimental.

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@promag promag force-pushed the 2019-06-scantxoutset-nits branch 2 times, most recently from 783f0e3 to e36bb00 Compare June 25, 2019 19:39
@promag
Copy link
Member Author

promag commented Jul 2, 2019

@jonasschnelli mind taking a look?

src/rpc/blockchain.cpp Show resolved Hide resolved
src/rpc/blockchain.cpp Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Jul 8, 2019

Concept ACK

Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

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

utACK e36bb00

}
bool res = FindScriptPubKey(g_scan_progress, g_should_abort_scan, count, pcursor.get(), needles, coins);
result.pushKV("success", res);
result.pushKV("searched_items", count);
result.pushKV("txouts", count);
result.pushKV("height", tip->nHeight);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe tipheight or bestblockheight?
But maybe height is also okay.

Copy link
Contributor

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

ACK e36bb00 modulo nits. Code review, compiled/ran help, inspected a few of the scantxoutset outputs in the functional test. Concrete CLI examples in the help would be nice to have.

" }\n"
" ,...], \n"
" \"total_amount\" : x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
" \"total_amount\": x.xxx, (numeric) The total amount of all found unspent outputs in " + CURRENCY_UNIT + "\n"
"]\n"
},
RPCExamples{""},
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to add one or two bitcoin-cli examples here.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you suggest?

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@laanwj
Copy link
Member

laanwj commented Sep 9, 2019

ACK bdd6a4f

I think this is enough of an improvement in itself, examples can be added later.

laanwj added a commit that referenced this pull request Sep 9, 2019
bdd6a4f qa: Check scantxoutset result against gettxoutsetinfo (João Barbosa)
fc0c410 rpc: Improve scantxoutset response and help message (João Barbosa)

Pull request description:

  The new response keys `height` and `bestblock` allow the client to know at what point the scan took place.

  The help message now has all the response keys (`result` and `txouts` were missing) and it's improved a bit. Note that `searched_items` key is renamed to `txouts`, considering `scantxoutset` is marked experimental.

ACKs for top commit:
  laanwj:
    ACK bdd6a4f

Tree-SHA512: 6bb7c3464b19857b756b8bc491ab7c58b0d948aad8c005b26ed27c55a1278f5639217e11a315bb505b4f44ebe86f413068c1e539c8a5f7a4007735586cc6443c
@laanwj laanwj merged commit bdd6a4f into bitcoin:master Sep 9, 2019
@promag promag deleted the 2019-06-scantxoutset-nits branch September 9, 2019 08:44
@jonatack
Copy link
Contributor

jonatack commented Sep 9, 2019

I think this is enough of an improvement in itself, examples can be added later.

Yes. I might have a look at adding examples.

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 8, 2020
Summary:
qa: Check scantxoutset result against gettxoutsetinfo (João Barbosa)
rpc: Improve scantxoutset response and help message (João Barbosa)

Pull request description:

  The new response keys `height` and `bestblock` allow the client to know at what point the scan took place.

  The help message now has all the response keys (`result` and `txouts` were missing) and it's improved a bit. Note that `searched_items` key is renamed to `txouts`, considering `scantxoutset` is marked experimental.

---

Backport of Core [[bitcoin/bitcoin#16285 | PR16285]]

Test Plan:
  ninja check check-functional

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7402
laanwj added a commit to bitcoin-core/gui that referenced this pull request Dec 7, 2021
1ed5681 rpc: add missing scantxoutset examples (Sebastian Falbesoner)

Pull request description:

  The scantxoutset RPC and its help text was at last improved in #16285, but it's still missing examples (see bitcoin/bitcoin#16285 (comment)).

  ~Note that the example descriptor used doesn't follow the developer guideline of using invalid bech32 addresses, as the RPC is not wallet-related and it's use-case is merely to look up state information (i.e. there is no danger of sending funds to a wrong address).~ For the sake of simplicity, the raw descriptor for an early coinbase payout address (block 9) is taken, i.e. it yields results even at an early stage of IBD. Happy to change that though if there are other suggestions.

ACKs for top commit:
  shaavan:
    reACK 1ed5681

Tree-SHA512: 057ad9ac0d019035bee2332440128de0ef08580bbeae80182ff74771beead3555c4bf7008071a97bbb6a8d85fb85d0f0754fb7941db2c5b755eae1ac9aa65318
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2021
1ed5681 rpc: add missing scantxoutset examples (Sebastian Falbesoner)

Pull request description:

  The scantxoutset RPC and its help text was at last improved in bitcoin#16285, but it's still missing examples (see bitcoin#16285 (comment)).

  ~Note that the example descriptor used doesn't follow the developer guideline of using invalid bech32 addresses, as the RPC is not wallet-related and it's use-case is merely to look up state information (i.e. there is no danger of sending funds to a wrong address).~ For the sake of simplicity, the raw descriptor for an early coinbase payout address (block 9) is taken, i.e. it yields results even at an early stage of IBD. Happy to change that though if there are other suggestions.

ACKs for top commit:
  shaavan:
    reACK 1ed5681

Tree-SHA512: 057ad9ac0d019035bee2332440128de0ef08580bbeae80182ff74771beead3555c4bf7008071a97bbb6a8d85fb85d0f0754fb7941db2c5b755eae1ac9aa65318
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
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.

None yet

8 participants