💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2695600516)
Thanks for the ping @glozow I will amend the suggestions in the wiki

💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978281970)
Note: `WriteSettingsFile()` throws if the `-nosettings` argument is set, but in that case we would never find a value for `upnp` above, and `settings_changed` won't be true, so that edge case is handled well.

💬 Crypt-iQ commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695634708)
Ah, I did not consider the minchainwork.

> So it shouldn't be possible to trigger this by sending header messages, but the two attacking peers could be be randomly selected next to each other.

I could have made that more clear in the description that the attacker needed two peers to be selected in sequence for the `SendMessages` call. That's what I meant by "subsequent" but I think it was unclear.

> dependent on already_in_flight - could we unconditionally skip out here if CanDirectFetch() re
...

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2695648258)
I have dropped the `QualityLevel::NEEDS_SPLIT_OPTIMAL` value, and made the "post-linearize after remove, before split" apply to `NEEDS_SPLIT_ACCEPTABLE` instead, with updated comments about it.

💬 instagibbs commented on issue "compact blocks in IBD resets m_stalling_since":
(https://github.com/bitcoin/bitcoin/issues/31962#issuecomment-2695653952)
That code block appears to be in since the dawn of compact blocks: https://github.com/bitcoin/bitcoin/commit/d25cd3ec4e8#diff-34d21af3c614ea3cee120df276c9c4ae95053830d7f1d3deaf009a4625409ad2R5312

I might be missing something of course.

> At least removing the condition doesn't break any existing tests.

Not very comforting :)

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978311245)
Yes, i used to have an explicit check around this logic for dynamic settings to be enabled (so that it won't throw), but removed it after a comment by @ryanofsky. It indeed doesn't seem to be necessary because the dynamic settings won't be populated in the first place.

💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1978313107)
Thanks, I missed that discussion. (https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965808053)

💬 laanwj commented on pull request "delete release note fragments for v29":
(https://github.com/bitcoin/bitcoin/pull/31976#issuecomment-2695681192)
ACK ae92bd8e1b2c144697662ba0358c27f5c1892bb2

💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2695688502)
lightly reviewed code, tested ACK https://github.com/bitcoin/bitcoin/commit/44041ae0eca9d2034b7c2bdef24724809cc35e90

There are a lot of possible combinations of settings in bitcoin.conf, commandline arguments, and GUI settings, I sanity tested the happy path of `upnp=true` in settings.json, and a few cases I thought would be tricky with `-nosettings` and `-upnp` as a cli argument and in bitcoin.conf, and this solution handles them all well, and I can't think of any unhandled cases from review
...

👍 laanwj approved a pull request: "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy"
(https://github.com/bitcoin/bitcoin/pull/31973#pullrequestreview-2655481086)
Code review ACK 65e503e8499f18e48d7801a298d9acdd6ed62d0b

💬 darosior commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1978342771)
> Curious to know the reason behind using UTx0 instead of UTXO.

:shrug: doesn't matter, but as it is an acronym it makes more sense to me to capitalize the first letter of each word an not the others.

> Can this be made more understandable?

That's a pretty vague request.. I tried to be as detailed as possible in the commit message since the change is not obvious. Could you point to what you don't understand exactly?

> What I can gather is this seems to make the test less tight aroun
...

💬 achow101 commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2695738356)
Could also do testnet4, my seeder publishes a seeds.txt.gz at https://testnet4.achownodes.xyz/seeds.txt.gz

💬 darosior commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1978352110)
> Also not sure why we need to add `VARINT(0)`?

You mean in the comment? I guess it doesn't matter either way but it's clearer this way...

If like @Prabhat1308 you ask why it's necessary to have an empty varint in the serialization itself, this is simply because otherwise you would get a serialization error instead of a `MoneyRange` error.

To give more details:
- this test is checking the error returned there https://github.com/bitcoin/bitcoin/blob/79bbb381a1fd13ad456fde736b3c195a791d4
...

💬 darosior commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1978352350)
See https://github.com/bitcoin/bitcoin/pull/31907/commits/dedc2d9cecccb866230c07112d00288237d12eaa#r1978352110

💬 achow101 commented on pull request "seeds: update makeseeds regex and DNS fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1978362605)
In 6597e87666daa7fabdf310e9ad39fc8a7c81ceea "makeseeds: fix incorrect regex"

Perhaps put the `|` in the front of every regex rather than the end, to make it easier to update in the future and not accidentally run into this problem again.

🤔 mzumsande reviewed a pull request: "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data"
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2655550047)
Code Review ACK 63b534f97e591d4e107fd5148909852eb2965d27

💬 achow101 commented on pull request "delete release note fragments for v29":
(https://github.com/bitcoin/bitcoin/pull/31976#issuecomment-2695782322)
ACK ae92bd8e1b2c144697662ba0358c27f5c1892bb2

🚀 achow101 merged a pull request: "delete release note fragments for v29"
(https://github.com/bitcoin/bitcoin/pull/31976)

💬 pablomartin4btc commented on pull request "Add assumeutxo chainparams to release-process.md":
(https://github.com/bitcoin/bitcoin/pull/31940#discussion_r1978380037)
Just for reference (_"hash_serialized_3 hash type cannot be queried for a specific block"_):
https://github.com/bitcoin/bitcoin/blob/afde95b4601e6ac44b9fa313747c28b656d1faf2/test/functional/feature_coinstatsindex.py#L296-L303

🤔 pablomartin4btc reviewed a pull request: "Add assumeutxo chainparams to release-process.md"
(https://github.com/bitcoin/bitcoin/pull/31940#pullrequestreview-2655571248)
reACK 9dcf4b6c4d183ab069459b9be49ef7acd7d3be4b