🤔 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,
...
💬 vijayabhaskar78 commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640783505)
@Prabhat1308 Thanks I will remove those extra spaces but I need to talk to you can you connect with me on LinkedIn here is my profile link : https://www.linkedin.com/in/svijayabhaskar?utm_source=share&utm_campaign=share_via&utm_content=profile&utm_medium=android_app I would like to talk about the issue, I have sent you the request.
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640783505)
@Prabhat1308 Thanks I will remove those extra spaces but I need to talk to you can you connect with me on LinkedIn here is my profile link : https://www.linkedin.com/in/svijayabhaskar?utm_source=share&utm_campaign=share_via&utm_content=profile&utm_medium=android_app I would like to talk about the issue, I have sent you the request.
💬 pinheadmz commented on pull request "Fix MSVC ccache reporting (Fixes #31771)":
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640787298)
@vijayabhaskar78 posting external social media links isn't really appropriate here.
(https://github.com/bitcoin/bitcoin/pull/31813#issuecomment-2640787298)
@vijayabhaskar78 posting external social media links isn't really appropriate here.