💬 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
...
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945268728)
There are two cases to this.
1. When the `-blockmaxweight` is default (`4,000,000 WU`), and you set a higher block reserved weight, the node will fail to start, there is a test for this in this PR
2. When `-blockmaxweight` is custom i.e lower that (`4,000,000 WU`) and you set a block reserved weight to a value
a) > `4,000,000WU` the node will fail to start (There is a test for this)
b) < `4000,000 WU` the node will start but all generated block template will be empty whe
...
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945268728)
There are two cases to this.
1. When the `-blockmaxweight` is default (`4,000,000 WU`), and you set a higher block reserved weight, the node will fail to start, there is a test for this in this PR
2. When `-blockmaxweight` is custom i.e lower that (`4,000,000 WU`) and you set a block reserved weight to a value
a) > `4,000,000WU` the node will fail to start (There is a test for this)
b) < `4000,000 WU` the node will start but all generated block template will be empty whe
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945269560)
It's explained in `BlockAssembler::Options ClampOptions`, though we should probably point to that from elsewhere.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945269560)
It's explained in `BlockAssembler::Options ClampOptions`, though we should probably point to that from elsewhere.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945270752)
Also see https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937951204
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945270752)
Also see https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1937951204
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945272414)
(our comments came in at the same time)
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945272414)
(our comments came in at the same time)
💬 glozow commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945280589)
Ok, makes sense to get rid of `-blockmaxweight`. What do you think the plan for that should be? If I understand you correctly that users should really only use one of them, should we be deprecating it at the same time as introducing `-blockreservedweight`?
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945280589)
Ok, makes sense to get rid of `-blockmaxweight`. What do you think the plan for that should be? If I understand you correctly that users should really only use one of them, should we be deprecating it at the same time as introducing `-blockreservedweight`?
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945283572)
I think we should deprecate it in next release and then remove it later.
Because ocean mining currently heavily rely on `-blockmaxweight`
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1945283572)
I think we should deprecate it in next release and then remove it later.
Because ocean mining currently heavily rely on `-blockmaxweight`
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945293526)
For example I tried this. I expected a performance boost but didn't really observe one:
```cpp
auto io_readiness{GenerateWaitSockets()};
if (io_readiness.events_per_sock.empty()) {
interruptNet.sleep_for(SELECT_TIMEOUT);
} else {
// WaitMany() may as well be a static method, the context of the first Sock in the vector is not relevant.
io_readiness.events_per_sock.begin()->first->WaitMany(SELECT_TIMEOUT,
...
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1945293526)
For example I tried this. I expected a performance boost but didn't really observe one:
```cpp
auto io_readiness{GenerateWaitSockets()};
if (io_readiness.events_per_sock.empty()) {
interruptNet.sleep_for(SELECT_TIMEOUT);
} else {
// WaitMany() may as well be a static method, the context of the first Sock in the vector is not relevant.
io_readiness.events_per_sock.begin()->first->WaitMany(SELECT_TIMEOUT,
...