✅ Vasu-08 closed an issue: "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit""
(https://github.com/bitcoin/bitcoin/issues/28250)
(https://github.com/bitcoin/bitcoin/issues/28250)
💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290949477)
> to enforces only expected types are passed at compile time
Do we want to revisit #22978 first before doing compile time type checking for RPC?
I think compile time RPC type checks would require templatising `RPCArg` by what is currently `m_type` (and `m_inner` perhaps), changing `RPCHelpMan` to be templatised based on a `std::tuple<...>` of `RPCArg<X>`s, and likewise for `RPCMethodImpl` which then gives the actual RPC implementation compile time access to its expected argument types.
...
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290949477)
> to enforces only expected types are passed at compile time
Do we want to revisit #22978 first before doing compile time type checking for RPC?
I think compile time RPC type checks would require templatising `RPCArg` by what is currently `m_type` (and `m_inner` perhaps), changing `RPCHelpMan` to be templatised based on a `std::tuple<...>` of `RPCArg<X>`s, and likewise for `RPCMethodImpl` which then gives the actual RPC implementation compile time access to its expected argument types.
...
💬 ChrisMartl commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1674299315)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1674299315)
Concept ACK
💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1291005941)
Yeah, I'll remove this line.
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1291005941)
Yeah, I'll remove this line.
⚠️ Jones098 opened an issue: "Release "
(https://github.com/bitcoin/bitcoin/issues/28256)
https://github.com/Jones098/Denero1987/issues/21#issue-1846360170
(https://github.com/bitcoin/bitcoin/issues/28256)
https://github.com/Jones098/Denero1987/issues/21#issue-1846360170
✅ willcl-ark closed an issue: "Release "
(https://github.com/bitcoin/bitcoin/issues/28256)
(https://github.com/bitcoin/bitcoin/issues/28256)
💬 Jones098 commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291012828)
Release
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291012828)
Release
💬 Symphonic3 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674343686)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674343686)
Concept ACK
:lock: fanquake locked an issue: "Release "
(https://github.com/bitcoin/bitcoin/issues/28256)
(https://github.com/bitcoin/bitcoin/issues/28256)
✅ fanquake closed an issue: "easy loading custom blockchains"
(https://github.com/bitcoin/bitcoin/issues/28255)
(https://github.com/bitcoin/bitcoin/issues/28255)
:lock: fanquake locked an issue: "easy loading custom blockchains"
(https://github.com/bitcoin/bitcoin/issues/28255)
(https://github.com/bitcoin/bitcoin/issues/28255)
💬 TheCharlatan commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674459315)
Thank you for the review @furszy
Updated cfe93642b27cbd4770200b9a6e571e1388ba35dc -> 3c1c434ae0f1e9649949a1bb27ea14c1dbad06cc ([contextSanityChecks_0](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_0) -> [contextSanityChecks_1](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_0..contextSanityChecks_1))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/2
...
(https://github.com/bitcoin/bitcoin/pull/28228#issuecomment-1674459315)
Thank you for the review @furszy
Updated cfe93642b27cbd4770200b9a6e571e1388ba35dc -> 3c1c434ae0f1e9649949a1bb27ea14c1dbad06cc ([contextSanityChecks_0](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_0) -> [contextSanityChecks_1](https://github.com/TheCharlatan/bitcoin/tree/contextSanityChecks_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/contextSanityChecks_0..contextSanityChecks_1))
* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/2
...
💬 TheCharlatan commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#issuecomment-1674461756)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28253#issuecomment-1674461756)
Concept ACK
💬 fanquake commented on pull request "doc: use llvm-config for bitcoin-tidy example":
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1674463539)
> Nit: stale description.
Updated the description.
(https://github.com/bitcoin/bitcoin/pull/28245#issuecomment-1674463539)
> Nit: stale description.
Updated the description.
🚀 fanquake merged a pull request: "doc: use llvm-config for bitcoin-tidy example"
(https://github.com/bitcoin/bitcoin/pull/28245)
(https://github.com/bitcoin/bitcoin/pull/28245)
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291125153)
@ajtowns I raised a similar concern, but it looks like this is safe: https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291125153)
@ajtowns I raised a similar concern, but it looks like this is safe: https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1285851447
💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291139542)
> Should we add an rpc to allow querying the orphanage?
Could do that, as there's a bit of black box-ness here. Though I also think that the ideal situation would be to write unit tests if we're interested in the exact contents of the orphanage.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1291139542)
> Should we add an rpc to allow querying the orphanage?
Could do that, as there's a bit of black box-ness here. Though I also think that the ideal situation would be to write unit tests if we're interested in the exact contents of the orphanage.
💬 BitcoinErrorLog commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674487742)
NACK.
The preceding mempoolfullrbf debate resulted in a Bitcoin Core where mempool policy is a minority voting system.
While I acknowledge that mempool policy is local and any consistency we have across policies is a phenomenon of incentives, AND that the likely proper solution is to REMOVE mempool policy steering from both the Core community and software, here we are nonetheless.
The default config for Core is a dangerous space to design, and we should hold the user space, and status
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1674487742)
NACK.
The preceding mempoolfullrbf debate resulted in a Bitcoin Core where mempool policy is a minority voting system.
While I acknowledge that mempool policy is local and any consistency we have across policies is a phenomenon of incentives, AND that the likely proper solution is to REMOVE mempool policy steering from both the Core community and software, here we are nonetheless.
The default config for Core is a dangerous space to design, and we should hold the user space, and status
...
🤔 glozow reviewed a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1573365456)
> I think we should probably prefer to test behaviours that are intentional/desirable, rather than behaviours that just happen to be how it got implemented.
Yeah the purpose of writing these tests was largely documentation and not changing behavior unintentionally.
I think most of the things here are intentional, though can think of at least one thing that seems to be "that's just how it got implemented." I was trying to figure out whether we would want to use a `notfound` for a parent tx
...
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1573365456)
> I think we should probably prefer to test behaviours that are intentional/desirable, rather than behaviours that just happen to be how it got implemented.
Yeah the purpose of writing these tests was largely documentation and not changing behavior unintentionally.
I think most of the things here are intentional, though can think of at least one thing that seems to be "that's just how it got implemented." I was trying to figure out whether we would want to use a `notfound` for a parent tx
...
💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291163628)
I'd be on board with that approach, too. A bit more explicit and reduces implementation complexity, while I think usability is as good, since we already require the user to distinguish between the arg having a default or not.
nit/bikeshed: I'd prefer `Arg` over `ArgOrDefault`.
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1291163628)
I'd be on board with that approach, too. A bit more explicit and reduces implementation complexity, while I think usability is as good, since we already require the user to distinguish between the arg having a default or not.
nit/bikeshed: I'd prefer `Arg` over `ArgOrDefault`.