💬 murchandamus commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1945108666)
Do I understand right that this test will try the whole battery of different script trees seen below and uses the maximal estimate across script trees, so if there are multiple different leaves that we can satisfy, our transaction might overpay, but no longer underpay?
(https://github.com/bitcoin/bitcoin/pull/26573#discussion_r1945108666)
Do I understand right that this test will try the whole battery of different script trees seen below and uses the maximal estimate across script trees, so if there are multiple different leaves that we can satisfy, our transaction might overpay, but no longer underpay?
💬 darosior commented on issue "nSequence is not set when spending from satisfiable descriptor with relative timelock":
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640490507)
> I have searched the existing issues
Hmm https://github.com/bitcoin/bitcoin/issues/27527. :)
> Transaction building may be setting nLockTime / nSequence values which aren't actually compatible with what we can sign for.
To add to this, the coin selection algorithm right now may even be selecting coins that are incompatible with each other: https://github.com/bitcoin/bitcoin/issues/27526.
(https://github.com/bitcoin/bitcoin/issues/31808#issuecomment-2640490507)
> I have searched the existing issues
Hmm https://github.com/bitcoin/bitcoin/issues/27527. :)
> Transaction building may be setting nLockTime / nSequence values which aren't actually compatible with what we can sign for.
To add to this, the coin selection algorithm right now may even be selecting coins that are incompatible with each other: https://github.com/bitcoin/bitcoin/issues/27526.
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945120612)
if `WaitMany()` returns false, we would have already waited the duration of `SELECT_TIMEOUT` -- do we need to `sleep_for()` again?
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945120612)
if `WaitMany()` returns false, we would have already waited the duration of `SELECT_TIMEOUT` -- do we need to `sleep_for()` again?
📝 vijayabhaskar78 opened a pull request: "Fix MSVC ccache reporting (Fixes #31771)"
(https://github.com/bitcoin/bitcoin/pull/31813)
Fixes #31771
### Summary
This PR resolves the incorrect reporting of `ccache` usage for MSVC builds by:
1. Removing forced compiler launcher configuration
2. Improving status message accuracy
3. Documenting proper MSVC+ccache workflows
### Key Changes
- **Deleted**: `cmake/ccache.cmake` (Eliminates conflicts with user/CI configurations like `sccache`)
- **Updated**: `CMakeLists.txt` status reporting
```cmake
if(CMAKE_CXX_COMPILER_LAUNCHER)
message(STATUS "Com
...
(https://github.com/bitcoin/bitcoin/pull/31813)
Fixes #31771
### Summary
This PR resolves the incorrect reporting of `ccache` usage for MSVC builds by:
1. Removing forced compiler launcher configuration
2. Improving status message accuracy
3. Documenting proper MSVC+ccache workflows
### Key Changes
- **Deleted**: `cmake/ccache.cmake` (Eliminates conflicts with user/CI configurations like `sccache`)
- **Updated**: `CMakeLists.txt` status reporting
```cmake
if(CMAKE_CXX_COMPILER_LAUNCHER)
message(STATUS "Com
...
💬 vijayabhaskar78 commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640653595)
@hebasto Can you help me regarding this PR please
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640653595)
@hebasto Can you help me regarding this PR please
💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945226453)
Or have a test for the expected behavior? A comment in the options-handling code? I don't think it should be unspecified.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945226453)
Or have a test for the expected behavior? A comment in the options-handling code? I don't think it should be unspecified.
💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945193198)
Should the PCP target include IPv6 or no? Something like `(domain == AF_INET || domain == AF_INET6)` might improve the test.
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945193198)
Should the PCP target include IPv6 or no? Something like `(domain == AF_INET || domain == AF_INET6)` might improve the test.
💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945195855)
I don't see this used anywhere.
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945195855)
I don't see this used anywhere.
💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945222662)
The `ConsumeData` here was the culprit for the bad coverage.
Basically, this function needs to set `name_len` to 16 or 28 for the fuzzer to continue past `SetSockAddr` in `PCPRequestPortMap`. In the PCP case, `name_len` started here as 128 and then `ConsumeData` would look to return the min of 128 or the remaining bytes of the input. It would consume the rest of the input every time because the fuzzer was optimizing for the `SetSockAddr` check. So, there would always be nothing left in `fuzz
...
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1945222662)
The `ConsumeData` here was the culprit for the bad coverage.
Basically, this function needs to set `name_len` to 16 or 28 for the fuzzer to continue past `SetSockAddr` in `PCPRequestPortMap`. In the PCP case, `name_len` started here as 128 and then `ConsumeData` would look to return the min of 128 or the remaining bytes of the input. It would consume the rest of the input every time because the fuzzer was optimizing for the `SetSockAddr` check. So, there would always be nothing left in `fuzz
...
🤔 marcofleon reviewed a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2599495421)
I ran `pcp_request_port_map` with the patch I discuss below and looks like coverage is good to go now.
https://marcofleon.github.io/coverage/pcp_request_port_map/coverage/root/bitcoin/src/common/pcp.cpp.html
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2599495421)
I ran `pcp_request_port_map` with the patch I discuss below and looks like coverage is good to go now.
https://marcofleon.github.io/coverage/pcp_request_port_map/coverage/root/bitcoin/src/common/pcp.cpp.html
💬 mzumsande commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640677459)
Given that this will likely make IBD a bit slower due to repeated checks, I think there should be more tangible benefits than just simpler reasoning about edge cases - or maybe it would be good to explain these edge cases a bit more to be able to judge how relevant they are.
Once we are synced to the tip, block acception and connection typically happen right after each other in `ProcessNewBlock()`, so I can only think of of rather far-fetched scenarios for the repeated checks to become releva
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640677459)
Given that this will likely make IBD a bit slower due to repeated checks, I think there should be more tangible benefits than just simpler reasoning about edge cases - or maybe it would be good to explain these edge cases a bit more to be able to judge how relevant they are.
Once we are synced to the tip, block acception and connection typically happen right after each other in `ProcessNewBlock()`, so I can only think of of rather far-fetched scenarios for the repeated checks to become releva
...
💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945227927)
Meh good point miner policy is policy, resolving.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945227927)
Meh good point miner policy is policy, resolving.
💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640695928)
This becomes a bit likelier in IBD, but i agree. As i [said yesterday on IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-02-05#1738786490-1738786524;) i am not sure this change is worth doing especially since we can never really solve the issue of a non-upgraded node having accepted invalid blocks short of re-connecting the whole chain when upgrading.
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640695928)
This becomes a bit likelier in IBD, but i agree. As i [said yesterday on IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-02-05#1738786490-1738786524;) i am not sure this change is worth doing especially since we can never really solve the issue of a non-upgraded node having accepted invalid blocks short of re-connecting the whole chain when upgrading.
💬 darosior commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640697895)
I'll try to bug @sdaftuar, who introduced this comment, to see if we are missing something.
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640697895)
I'll try to bug @sdaftuar, who introduced this comment, to see if we are missing something.
💬 Prabhat1308 commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#discussion_r1945244783)
There are a lot of changes in this file that are not required . Please adhere to not changing the code if not necessary. Please remove the extra whitespaces and syntax highlighting.
(https://github.com/bitcoin/bitcoin/pull/31813#discussion_r1945244783)
There are a lot of changes in this file that are not required . Please adhere to not changing the code if not necessary. Please remove the extra whitespaces and syntax highlighting.
💬 Prabhat1308 commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#discussion_r1945245975)
Similar irrelevant changes introduced in this file .
(https://github.com/bitcoin/bitcoin/pull/31813#discussion_r1945245975)
Similar irrelevant changes introduced in this file .
💬 Sjors commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945251300)
Just the line you're touching is fine, yes. Thanks.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945251300)
Just the line you're touching is fine, yes. Thanks.
💬 Sjors commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945252447)
"example given" is how I remember e.g.
(https://github.com/bitcoin/bitcoin/pull/31805#discussion_r1945252447)
"example given" is how I remember e.g.
💬 sr-gi commented on pull request "test: test_inv_block, use mocktime instead of waiting":
(https://github.com/bitcoin/bitcoin/pull/31811#issuecomment-2640735224)
tACK [2706c5b](https://github.com/bitcoin/bitcoin/pull/31811/commits/2706c5b7c8eee7ffd8c3b23a8012f346165ddb93)
Run the test several times and it seems to be consistent around ~30s now, whereas it previously fluctuated between ~30-90s
(https://github.com/bitcoin/bitcoin/pull/31811#issuecomment-2640735224)
tACK [2706c5b](https://github.com/bitcoin/bitcoin/pull/31811/commits/2706c5b7c8eee7ffd8c3b23a8012f346165ddb93)
Run the test several times and it seems to be consistent around ~30s now, whereas it previously fluctuated between ~30-90s
💬 Sjors commented on pull request "Double check all block rules in `ConnectBlock`, not only `CheckBlock`":
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640736710)
> short of re-connecting the whole chain when upgrading
As a generic method that's the only way. And it's a potentially big burden, especially for some reason unwinding is much slower than IBD (last time I tried).
But for specific soft-forks there maybe quicker ways. E.g. it's easy to scan the headers for timewarp violations. Checking coinbases for the new proposed BIP34 rule is only possible for non-pruned blocks, but would be quick.
The new proposed sigops check on the other hand prob
...
(https://github.com/bitcoin/bitcoin/pull/31792#issuecomment-2640736710)
> short of re-connecting the whole chain when upgrading
As a generic method that's the only way. And it's a potentially big burden, especially for some reason unwinding is much slower than IBD (last time I tried).
But for specific soft-forks there maybe quicker ways. E.g. it's easy to scan the headers for timewarp violations. Checking coinbases for the new proposed BIP34 rule is only possible for non-pruned blocks, but would be quick.
The new proposed sigops check on the other hand prob
...