💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1468209943)
Yeah, thanks for the suggestion. Reviewers may be interested in https://github.com/sipa/miniscript/pull/134 as well.
> Also just checking if this PR should be in Draft or not? Are there ongoing open discussions on anything important or is this ready for extensive review?
It is ready for (extensive or not) review. Otherwise i'd have opened it as a draft. But please tell me clearly if you disagree.
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1468209943)
Yeah, thanks for the suggestion. Reviewers may be interested in https://github.com/sipa/miniscript/pull/134 as well.
> Also just checking if this PR should be in Draft or not? Are there ongoing open discussions on anything important or is this ready for extensive review?
It is ready for (extensive or not) review. Otherwise i'd have opened it as a draft. But please tell me clearly if you disagree.
💬 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)?
(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`?
(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
...
(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?
(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)
(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)
(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.";
```
(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?
(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.
(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.
(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
(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).
...
(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
...
(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).
(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?
(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
(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
...
(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`?
(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
(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