💬 fanquake commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135405169)
Pretty sure this can just be merged into the includes below.
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135405169)
Pretty sure this can just be merged into the includes below.
💬 fanquake commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135406000)
I think include fixing is fine. However theres no need to include unrelated clang-format changes.
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135406000)
I think include fixing is fine. However theres no need to include unrelated clang-format changes.
💬 MarcoFalke commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135406385)
You'll need to set `-U0` for the context to avoid unrelated changes, no?
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135406385)
You'll need to set `-U0` for the context to avoid unrelated changes, no?
💬 fanquake commented on pull request "refactor: Split util/system into exception, shell, and fs-specific files":
(https://github.com/bitcoin/bitcoin/pull/25152#issuecomment-1467936449)
Going to close this for now, given that it's being split out, and there's a [thumbs up from the author above](https://github.com/bitcoin/bitcoin/pull/25152#issuecomment-1458326647).
(https://github.com/bitcoin/bitcoin/pull/25152#issuecomment-1467936449)
Going to close this for now, given that it's being split out, and there's a [thumbs up from the author above](https://github.com/bitcoin/bitcoin/pull/25152#issuecomment-1458326647).
✅ fanquake closed a pull request: "refactor: Split util/system into exception, shell, and fs-specific files"
(https://github.com/bitcoin/bitcoin/pull/25152)
(https://github.com/bitcoin/bitcoin/pull/25152)
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135417534)
I'm formatting each commit with `git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v`. I think I must have included this unrelated change somewhere. Will fix.
(https://github.com/bitcoin/bitcoin/pull/27254#discussion_r1135417534)
I'm formatting each commit with `git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v`. I think I must have included this unrelated change somewhere. Will fix.
💬 0xB10C commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1467960000)
> > 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.
> So what would be your approach to do it externally then? Especially if you want to get this information more often.
You can query the `getrawmempool` RPC with the verbose
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1467960000)
> > 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.
> So what would be your approach to do it externally then? Especially if you want to get this information more often.
You can query the `getrawmempool` RPC with the verbose
...
💬 TheCharlatan commented on pull request "refactor: Extract util/fs from util/system":
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1467983104)
Thanks for the review!
Updated b67efe778d4e4ad359fb8dccea7f92e17d1b1f95 -> ea278094e9937bf96157161941d38a0c08a0aa4e ([splitSystemFs_0](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_0) -> [splitSystemFs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_0..splitSystemFs_1)) to address review comments.
(https://github.com/bitcoin/bitcoin/pull/27254#issuecomment-1467983104)
Thanks for the review!
Updated b67efe778d4e4ad359fb8dccea7f92e17d1b1f95 -> ea278094e9937bf96157161941d38a0c08a0aa4e ([splitSystemFs_0](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_0) -> [splitSystemFs_1](https://github.com/TheCharlatan/bitcoin/commits/splitSystemFs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/splitSystemFs_0..splitSystemFs_1)) to address review comments.
💬 willcl-ark commented on pull request "wallet: GetEffectiveBalance":
(https://github.com/bitcoin/bitcoin/pull/26365#issuecomment-1467991659)
Going to close this as there are other, cleaner and better-maintained version(s) of it being worked on now.
(https://github.com/bitcoin/bitcoin/pull/26365#issuecomment-1467991659)
Going to close this as there are other, cleaner and better-maintained version(s) of it being worked on now.
✅ willcl-ark closed a pull request: "wallet: GetEffectiveBalance"
(https://github.com/bitcoin/bitcoin/pull/26365)
(https://github.com/bitcoin/bitcoin/pull/26365)
💬 virtu commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1135459251)
Last time I checked event names could not be reused, so the package rejection code would have to use a new event.
On the bright side, having separate rejection events might help differentiating between them and avoid conflating regular vs. package-based rejections.
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1135459251)
Last time I checked event names could not be reused, so the package rejection code would have to use a new event.
On the bright side, having separate rejection events might help differentiating between them and avoid conflating regular vs. package-based rejections.
📝 darosior opened a pull request: "MiniTapscript: port Miniscript to Tapscript"
(https://github.com/bitcoin/bitcoin/pull/27255)
Miniscript was targeting P2WSH, and as such can currently only be used in `wsh()` descriptors. This pull request introduces support for Tapscript in Miniscript and makes Miniscript available inside `tr()` descriptors. It adds support for both watching *and* signing TapMiniscript descriptors.
The main changes to Miniscript for Tapscript are the following:
- A new `multi_a` fragment is introduced with the same semantics as `multi`. Like in other descriptors `multi` and `multi_a` can exclusivel
...
(https://github.com/bitcoin/bitcoin/pull/27255)
Miniscript was targeting P2WSH, and as such can currently only be used in `wsh()` descriptors. This pull request introduces support for Tapscript in Miniscript and makes Miniscript available inside `tr()` descriptors. It adds support for both watching *and* signing TapMiniscript descriptors.
The main changes to Miniscript for Tapscript are the following:
- A new `multi_a` fragment is introduced with the same semantics as `multi`. Like in other descriptors `multi` and `multi_a` can exclusivel
...
💬 glozow commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1135469419)
Mm good to know, thanks!
(https://github.com/bitcoin/bitcoin/pull/26531#discussion_r1135469419)
Mm good to know, thanks!
💬 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`.