💬 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.
💬 i-am-yuvi commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2640833530)
re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2640833530)
re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53
💬 davidgumberg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1945326195)
This branch is unused, maybe delete if retouch
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1945326195)
This branch is unused, maybe delete if retouch
💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1945331641)
I wonder if this should be an `add_custom_command()` with a [`POST_BUILD` event](https://cmake.org/cmake/help/latest/command/add_custom_command.html#build-events) instead? Presumably this version (I haven't checked) creates the symlink even if the link fails, whereas I would expect a `POST_BUILD`command to not have that problem.
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1945331641)
I wonder if this should be an `add_custom_command()` with a [`POST_BUILD` event](https://cmake.org/cmake/help/latest/command/add_custom_command.html#build-events) instead? Presumably this version (I haven't checked) creates the symlink even if the link fails, whereas I would expect a `POST_BUILD`command to not have that problem.
👍 theuni approved a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2599736642)
utACK 433aa99d238740bbdcb07b53b17f639170bd326d
Only change since my last review is the symlink script, which I don't feel strongly about either way. I left a comment about a fixup for it, but it's not a blocker for me.
(https://github.com/bitcoin/bitcoin/pull/31161#pullrequestreview-2599736642)
utACK 433aa99d238740bbdcb07b53b17f639170bd326d
Only change since my last review is the symlink script, which I don't feel strongly about either way. I left a comment about a fixup for it, but it's not a blocker for me.
💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945336859)
That doesn't seem likely to be true? Do you mean `>= m_orphans.size()`?
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945336859)
That doesn't seem likely to be true? Do you mean `>= m_orphans.size()`?
💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945338874)
oops, yes
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945338874)
oops, yes
💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945342636)
Moved out of the loop
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945342636)
Moved out of the loop