-
Notifications
You must be signed in to change notification settings - Fork 37.8k
rpc: getblock/getrawtransaction/decode*/gettxout fixups #24718
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
{RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"}, | ||
{RPCResult::Type::STR, "desc", "Inferred descriptor for the script"}, | ||
{RPCResult::Type::STR, "type", "The type (one of: " + GetAllOutputTypes() + ")"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think " of the script public key" is relevant here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think " of the script public key" is relevant here
Thanks, Luke, I'll keep that in mind when revisiting this.
What's the status of this? Have the changes been re-verified? Drahtbot has reported the conflicts at least one time. Looking at the changes, it seems they could be consolidated. Commits like 96b3a9b and 68a041d are doing the same refactor of changing |
Yes, this started out as fixes for one RPC, then I saw similar ones across others, and asked myself whether people prefer changes by kind or by RPC, which is another reason it was in draft. I plan to go through this with fresh eyes and either break it down to changes for one RPC or for one kind of change in one or more small, easy-to-review pulls so they don't just sit there for months. |
I don't think this needs to be made any more granular. What is the motivation for per-RPC commits?
If you're referencing this PR; it's been in draft since it was opened, and needed rebase for ~ 1 month. So it's not super surprising that it hasn't had much interest / review. |
I'll re-open this differently. It's not going to see any review after the comments here. |
Github-Pull: bitcoin#24718 Rebased-From: 2a4767f
Github-Pull: bitcoin#24718 Rebased-From: ffe6a9f
Github-Pull: bitcoin#24718 Rebased-From: 758b7cf
Github-Pull: bitcoin#24718 Rebased-From: 716c3fa
Github-Pull: bitcoin#24718 Rebased-From: 96b3a9b
Github-Pull: bitcoin#24718 Rebased-From: 68a041d
…getblock help 56d9244 RPC: Document "asm" and "hex" fields for scripts (Luke Dashjr) 2cdd4df Bugfix: RPC/blockchain: Correct type of "value" in getblock docs; add missing "desc" (Jon Atack) Pull request description: Inspired by #24718 ACKs for top commit: kristapsk: cr utACK 56d9244 Tree-SHA512: 2c6d0291397929f6a76b2d2998789187da123d7bfcace77375331cb81995eb0afd2600286c1e25cf68d16e35bd58706d2f672f63a3febe5e3a556a668f2175a2
In draft to re-verify the changes and see Drahtbot-enabled crossover with other pulls.