Skip to content

Conversation

jonatack
Copy link
Member

In draft to re-verify the changes and see Drahtbot-enabled crossover with other pulls.

@DrahtBot
Copy link
Contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24716 (rpc: Fix documentation assertion for getrawtransaction by laanwj)
  • #24661 (refactor: Use clang-tidy syntax for C++ named arguments by fanquake)
  • #23599 (Tidy up RPCTxSerializationFlags by MarcoFalke)
  • #23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)

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

🐙 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() + ")"},
Copy link
Member

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

Copy link
Member Author

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.

@fanquake
Copy link
Member

In draft to re-verify the changes and see Drahtbot-enabled crossover with other pulls.

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 "The type, eg 'pubkeyhash'" to "The type (one of: " + GetAllOutputTypes() + ")" (also in other commits). It probably makes sense to do that all in a single change, rather than spread across 5 commits.

@jonatack
Copy link
Member Author

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.

@fanquake
Copy link
Member

and either break it down to changes for one RPC or for one kind of change in one or more small

I don't think this needs to be made any more granular. What is the motivation for per-RPC commits?

easy-to-review pulls so they don't just sit there for months.

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.

@jonatack
Copy link
Member Author

I'll re-open this differently. It's not going to see any review after the comments here.

@jonatack jonatack closed this Apr 29, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
Github-Pull: bitcoin#24718
Rebased-From: 2a4767f
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
Github-Pull: bitcoin#24718
Rebased-From: 758b7cf
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
Github-Pull: bitcoin#24718
Rebased-From: 68a041d
maflcko pushed a commit that referenced this pull request Jul 25, 2022
…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
@bitcoin bitcoin locked and limited conversation to collaborators Apr 29, 2023
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.

4 participants