💬 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
...

💬 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.

💬 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

💬 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)

💬 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`?

💬 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`

💬 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,

...

💬 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.

💬 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.

💬 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

💬 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

💬 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.

👍 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.

💬 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()`?

💬 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

💬 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

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945343151)
(I added that instead)

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945346739)
So `m_total_orphan_weight`, `PeerOrphanInfo::m_total_weight`?

Are `BytesFromPeer` and `TotalOrphanBytes` ok? Or do we want `WeightFromPeer` and `TotalOrphanWeight`? Or maybe `UsageByPeer` and `TotalOrphanUsage`?

💬 furszy commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2640881605)
Concept ACK. Will review soon.

🤔 Prabhat1308 reviewed a pull request: "test: test_inv_block, use mocktime instead of waiting"
(https://github.com/bitcoin/bitcoin/pull/31811#pullrequestreview-2599781783)
tACK [2706c5b](https://github.com/bitcoin/bitcoin/pull/31811/commits/2706c5b7c8eee7ffd8c3b23a8012f346165ddb93)

Speedup is observed when running the test with the diffs.

ACK with the use of Mocktime as the test is independent from the use of wall-clock time. This can be seen by the use of `setmocktime()` in other functions in the same file as well .

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1945358694)
I like `Usage` :+1: