💬 furszy commented on pull request "kernel: Run sanity checks on context construction":
(https://github.com/bitcoin/bitcoin/pull/28228#discussion_r1290848071)
Isn't this logged in the caller as well?

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1674171648)
@luke-jr there was discussion [here](https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1173964808) regarding whether to keep the RPC hidden/public.

it was kept as a hidden RPC because:
1. a normal user wouldn't be interested in the new/tried table breakdown of addresses.
2. `-generate CLI` currently [uses](https://github.com/bitcoin/bitcoin/blob/467fa8943801911c233cb96d45282b1de10bfa90/src/bitcoin-cli.cpp#L1178) a [hidden RPC](https://github.com/bitcoin/bitcoin/blob/467fa8943801911
...

📝 stratospher converted_to_draft a pull request: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988)
implements https://github.com/bitcoin/bitcoin/issues/26907. built on top of https://github.com/bitcoin/bitcoin/pull/27511.

Rework of `-addrinfo` CLI is done using `getaddrmaninfo` RPC proposed in https://github.com/bitcoin/bitcoin/pull/27511. This would be useful for users who want to know the total number of addresses the node knows about and can make connections to.

Currently, `-addrinfo` returns total number of addresses the node knows about after filtering them for quality + recency
...

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

🤔 ariard reviewed a pull request: "validation: avoid mempool eviction mid-package evaluation"
(https://github.com/bitcoin/bitcoin/pull/28251#pullrequestreview-1572955374)
Note the code path affected by the bug is under the `submitpackage` RPC, which is clearly indicated as an experimental RPC with an unstable interface.

More test coverage sounds good.

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