💬 mzumsande commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1135648966)
I think that both removing it and adding an explanatory comment would be ok. Maybe the test could also check that the block statistics (`CBlockFileInfo`) is not the same after the call (`nBlocks` should have increased)?

💬 mzumsande commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1135688193)
As far as I know, test-only RPCs, similar to debug-only bitcoind options, aren't usually mentioned in release notes - the target group for release notes are regular users, while test-only options are meant for devs and may not be stable. Related test-only RPCs such as `addpeeraddress` weren't introduced in release notes either. So maybe only describe the changed behavior of cli -addrinfo and don't mention `getaddrmaninfo`?

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1468263557)
This PR removes the witness script push accounting in the Miniscript stack size checks, so as it stands it would actually make it possible to import a `wsh()` descriptor that would be unspendable by standardness since we currently only check whether the underlying Miniscript `IsSane()`.

The same holds for `tr()` descriptors generally, as a Miniscript may well be `IsSane()` on its own but not spendable if buried deep enough in a Taproot tree.

Both could be checked at parsing time, but it al
...

💬 ajtowns commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135671989)
I don't follow why this is being changed when `SelectParams()` is still used -- `SelectParams` already relies on `gArgs` and creates its own `unique_ptr<CChainParams>` so this is just creating two copies of the same object?

💬 ajtowns commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135678911)
Closer to a move-only commit if you didn't add the `CChainParams::` qualifier here (and for regtest)

💬 stickies-v commented on pull request "refactor: RPC: pass named argument value as string_view":
(https://github.com/bitcoin/bitcoin/pull/26612#issuecomment-1468266453)
> @stickies-v will you follow up here?

Done in https://github.com/bitcoin/bitcoin/pull/27256 (cc @ryanofsky)

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1135699239)
```suggestion
strPrint += "\nRun \"bitcoin-cli -h\" for help or \"bitcoin-cli listwallets\" to see which wallets are currently loaded.";
```

💬 instagibbs commented on pull request "[POLICY] Ephemeral anchors":
(https://github.com/bitcoin/bitcoin/pull/26403#issuecomment-1468273207)
Should a flag be threaded through to disallow any dust outputs even if they're EA?

💬 andrewtoth commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-1468285386)
ACK 0fe610deba336f0370d68c130dc9a223b7db964e modulo the dangling closing parenthesis in the error message.
Also, the first line of the commit message should be at most 80 chars.

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135718835)
This is a demo binary, so I think the point is to show that the `chainman` can now be instantiated with a `chainparams` that is itself instantiated without directly relying on `gArgs`. I'm not sure if this really makes sense though.

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1135725988)
Same as https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1042464406

💬 kiminuo commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1468344458)
@0xB10C

> > > Could a tracepoints approach, similar to #26531, work?
> >
> >
> > I need to research that in detail but it looks promising. Thank you for sharing it here!
>
> I don't think tracepoints are suited for this use-case. Tracepoints allow you to react to a specific event. Here, you request / pull data when a consumer needs it.

Tracepoints would be nice if I need to compute the histograms at specified intervals and fast (to avoid getting complete mempool from a full node).
...

💬 TheCharlatan commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#issuecomment-1468348476)
Updated 45048c0d7e0ee12210d226239fb5ce63d9ed4441 -> 600ab02bf58e073a93936438a7e884b3a7110f1c ([tc/2022-09-libbitcoinkernel-chainparams-args_6](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_6) -> [tc/2022-09-libbitcoinkernel-chainparams-args_7](https://github.com/TheCharlatan/bitcoin/commits/tc/2022-09-libbitcoinkernel-chainparams-args_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/tc/2022-09-libbitcoinkernel-chainparams-args_6..tc/202
...

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1135765960)
Could use [`std::visit`](https://en.cppreference.com/w/cpp/utility/variant/visit).

💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135849437)
'assert_raises_rpc_error' could be used here, but wouldn't that introduce another race condition where this assertion will fail if the rescan has already been completed?

💬 amitiuttarwar commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1135857860)
ah that makes sense. thanks

💬 ryanofsky commented on pull request "refactor / kernel: Move non-gArgs chainparams functionality to kernel":
(https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135870886)
re: https://github.com/bitcoin/bitcoin/pull/26177#discussion_r1135671989

> This is a demo binary, so I think the point is to show that the `chainman` can now be instantiated with a `chainparams` that is itself instantiated without directly relying on `gArgs`. I'm not sure if this really makes sense though.

IMO, it would be a little clearer if the vague "SETUP: Misc Globals" comment was changed to say specifically that SelectParams() call was needed to set some globals used by validation co
...

💬 MarcoFalke commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1135872769)
Right. Maybe just a `try`-`catch`-`pass`?

👍 ryanofsky approved a pull request: "refactor / kernel: Move non-gArgs chainparams functionality to kernel"
(https://github.com/bitcoin/bitcoin/pull/26177)
Code review ACK 600ab02bf58e073a93936438a7e884b3a7110f1c. Only changes since last review were rebase, and making suggested changes to make the split commit move only

💬 pablomartin4btc commented on pull request "cli: add validation to cli side commands besides when it's used with -rpcwallet":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1135879724)
fixed