💬 ariard commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290865427)
I think this could document more where the “do not LimitMempool” is applied, i.e in `Finalize()` L1123 which is itself called by `AcceptSingleTransaction` and the source of the bug. `LimitMempoolSize()` is called few times in validation.

💬 ariard commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290866687)
I think this check doesn’t have test coverage as tx are individually validated in `AcceptPackage`.

💬 ariard commented on pull request "validation: avoid mempool eviction mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#discussion_r1290857304)
The “disabled RBF” can sound as confusing. There is no nsequence opted-out of the transaction candidate targeted by a replacement, it sounds rather to mean the `m_allow_replacement` of `ATMPArgs` which is true for `SingleInPackageAccept` and false for `PackageChildWithParents`.

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1290884007)
done. added some more detail to the part about new table.

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1290884028)
done

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1290884145)
done

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1290884179)
done

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1674212552)
thanks @amitiuttarwar! addressed https://github.com/bitcoin/bitcoin/pull/27511#issuecomment-1657054147 to have better documentation for new/tried table. `getaddrmaninfo`'s help looks like this now:

```
getaddrmaninfo

Provides information about the node's address manager by returning the number of addresses in the `new` and `tried` tables and their sum for all networks.
This RPC is for testing only.

Result:
{ (json object) json object with network type as keys
"ne
...

💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290879367)
Isn't this buggy? We can have multiple RPC threads, making multiple requests to the same function, but that function only has a single `RPCHelpMan` which will simultaneously have different things assigned to `m_fun`?

🤔 ajtowns reviewed a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1572976655)
Concept ACK

Here's a branch demoing alternatives for the comments below: https://github.com/ajtowns/bitcoin/commits/202308-rpchelpfulreq

💬 ajtowns commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1290875538)
Saying `Arg<int*>(5)` and getting back a `std::optional<int>` instead of an `int*` seems weird? Why not just have two functions: `ArgOrNot<int>(5)` and `ArgOrDefault<int>(5)` and drop the outer `if constexpr` ?

💬 ajtowns commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1674237747)
If we're not differentiating between `OP_VER` and `OP_VERIF`, I think we should just merge the `DISABLED_OPCODE` and `BAD_OPCODE` errors.

💬 ajtowns commented on pull request "p2p: outbound network diversity refactoring and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1674241624)
This mostly seems like pointless refactoring (changing a `switch` to an `||` of two functions that themselves have switches). Doesn't seem worth the review effort to me.

💬 ChrisCho-H commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1674247997)
If `DISABLED_OPCODE` and `BAD_OPCODE` error throw same message, do we need to have both case?

💬 Vasu-08 commented on issue "RPC `createmultisig` outputs a `sh(addr(...))` descriptor when address_type field is "p2sh-segwit"":
(https://github.com/bitcoin/bitcoin/issues/28250#issuecomment-1674266366)
Ohh yeah, Thanks. Issue resolved. Closing it now.

✅ 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)

💬 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.


...

💬 ChrisMartl commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(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.

⚠️ Jones098 opened an issue: "Release "
(https://github.com/bitcoin/bitcoin/issues/28256)
https://github.com/Jones098/Denero1987/issues/21#issue-1846360170