💬 michaelfolkson commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1468032095)
Concept ACK, an important piece in opening up Taproot scripting.
> The design of Miniscript for Tapscript is the result of discussions between various people over the past year(s).
Your PR description is great but perhaps you can link to a few of those discussions for extra background reading for those that are interested? e.g. https://gist.github.com/sipa/06c5c844df155d4e5044c2c8cac9c05e
Also just checking if this PR should be in Draft or not? Are there ongoing open discussions on anyt
...
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1468032095)
Concept ACK, an important piece in opening up Taproot scripting.
> The design of Miniscript for Tapscript is the result of discussions between various people over the past year(s).
Your PR description is great but perhaps you can link to a few of those discussions for extra background reading for those that are interested? e.g. https://gist.github.com/sipa/06c5c844df155d4e5044c2c8cac9c05e
Also just checking if this PR should be in Draft or not? Are there ongoing open discussions on anyt
...
💬 ryanofsky commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1468037295)
Agree these functions should not be combined into existing `src/fs.h` since they are doing higher level things than accessing the filesystem. But instead of having two `fs.h` files in different locations, I would move `src/fs.h` to `src/util/fs.h` and move these functions to `src/util/fs_misc.h` or another place like that.
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1468037295)
Agree these functions should not be combined into existing `src/fs.h` since they are doing higher level things than accessing the filesystem. But instead of having two `fs.h` files in different locations, I would move `src/fs.h` to `src/util/fs.h` and move these functions to `src/util/fs_misc.h` or another place like that.
💬 kristapsk commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1468044559)
@0xB10C In general I agree it's better to keep Core simple if things can be done with some simple workarounds using existing APIs, but I feel this is something that could make some use in Qt GUI too, and for that reason it makes sense to implement inside Core codebase.
> IIRC there was also an out-of-band push to review this PR as "rumor has it that a well-known wallet is currently using Bitcoin Knots solely because this feature isn't available in this RPC interface (https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1468044559)
@0xB10C In general I agree it's better to keep Core simple if things can be done with some simple workarounds using existing APIs, but I feel this is something that could make some use in Qt GUI too, and for that reason it makes sense to implement inside Core codebase.
> IIRC there was also an out-of-band push to review this PR as "rumor has it that a well-known wallet is currently using Bitcoin Knots solely because this feature isn't available in this RPC interface (https://github.com/bitcoi
...
💬 ryanofsky commented on issue "untrust vs unsafe ?":
(https://github.com/bitcoin/bitcoin/issues/18625#issuecomment-1468052808)
> If we aren't going to change this then perhaps we can close the issue.
Even if not going to change RPC return values, information here shouldn't go to waste. It would be good to keep this open until:
1-Inconsistent names are fixed in the C++ code that this references
2-RPC documentation calls out fields with inconsistent names
Also, it should not be a breaking change to rename RPC parameters, because it is easy to add aliases for old names. But it would be a breaking change to rename
...
(https://github.com/bitcoin/bitcoin/issues/18625#issuecomment-1468052808)
> If we aren't going to change this then perhaps we can close the issue.
Even if not going to change RPC return values, information here shouldn't go to waste. It would be good to keep this open until:
1-Inconsistent names are fixed in the C++ code that this references
2-RPC documentation calls out fields with inconsistent names
Also, it should not be a breaking change to rename RPC parameters, because it is easy to add aliases for old names. But it would be a breaking change to rename
...
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1135537203)
No, this PR changes it to allow outgoing as well
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1135537203)
No, this PR changes it to allow outgoing as well
💬 pinheadmz commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible?":
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1468132225)
> Yes, this is the default value already.
Oh right -- why then does a second `sync_with_ping()` fix the race? Should `if wait_for_verack` wait for the node's ping before sending its own back?
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1468132225)
> Yes, this is the default value already.
Oh right -- why then does a second `sync_with_ping()` fix the race? Should `if wait_for_verack` wait for the node's ping before sending its own back?
💬 MarcoFalke commented on pull request "test: Make the unlikely race in p2p_invalid_messages impossible?":
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1468152330)
> Should if wait_for_verack wait for the node's ping before sending its own back?
Why? `ping` is sent after the `verack`.
(https://github.com/bitcoin/bitcoin/pull/27212#issuecomment-1468152330)
> Should if wait_for_verack wait for the node's ping before sending its own back?
Why? `ping` is sent after the `verack`.
📝 stickies-v opened a pull request: "refactor: rpc: remove ParseNonRFCJSONValue()"
(https://github.com/bitcoin/bitcoin/pull/27256)
As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly:
> IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. @laanwj pointed this out in https://github.com/bitcoin/bitcoin/issues/9028#issuecomment-257885368
The implementation of `ParseNonRFCJSONValue()` was already [simplified i
...
(https://github.com/bitcoin/bitcoin/pull/27256)
As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly:
> IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. @laanwj pointed this out in https://github.com/bitcoin/bitcoin/issues/9028#issuecomment-257885368
The implementation of `ParseNonRFCJSONValue()` was already [simplified i
...
💬 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