💬 glozow commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1964333834)
same

📝 theuni opened a pull request: "build: create Depends build type for depends and use it by default for depends builds"
(https://github.com/bitcoin/bitcoin/pull/31920)
As discussed a good bit with fanquake and hebasto. Would be nice to have for v29, but it's very late, so no worries if it doesn't make it.

Fundamentally, this creates a "Depends" build type which represents the flags that were used to build depends as opposed to colliding with the other types.

This allows the forwarding of optional flags into the CMake build for the "Depends" build type, but the user can now optionally use an existing (Debug/RelWitDebInfo/etc.) type to ignore the optimizat
...

💬 maflcko commented on pull request "build: create Depends build type for depends and use it by default for depends builds":
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-2672701261)
How does this interact with the depends DEBUG build? It looks like this is a breaking change for people (and the ci) picking the Debug build type afterwards. It would be good to clarify this and also adjust the CI scripts, if needed.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1964238336)
If I understand it correctly, the current logic is that we assign additional weight for all peers that have an entry in `m_peer_orphanage_info`, which means we don't assign weight for peers not participating in tx relay or that do but never sent us an orphan before. But once they sent us an orphan, that weight will stay reserved until they disconnect (and can be used by other peers too), even if they never send us another orphan.

This seems like a middle-ground between assigning each peer a f
...

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1964227057)
This is a bit ambiguous, I first read it as "don't erase the peer as an announcer if it was the only one", but what is meant is "erase the peer as an announcer, also remove the orphan if the peer was the only announcer", so maybe reword it.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1964296427)
One possibility is that an attacker who wants to spam us with orphans can do so via other peers, without providing the orphans themselves:

1.) Attacker A inv's a parent P to victim V but doesn't answer `GETDATA` (2 minutes timeout)
2.) Within these 2 minutes, A sends P and multiple non-conflicting children C to the rest of the network - these are accepted/relayed by all other peers, so will eventually be announced to V by all of its legitimate peers.
3.) All of V's legitimate peers will an
...

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1964220595)
"Remove a random announcement from this peer." seems better because we only sometimes evict an orphan.

💬 maflcko commented on pull request "test: cover edge case for lockunspent rpc":
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2672726545)
I am happy to review a pull request with the suggested fix in https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2457266568, but I don't have the time to work on this myself.

💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1964367079)
Fixed nit

💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1964367218)
Fixed nit

💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1964367337)
Fixed nit

💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1964369438)
Slightly surprisingly to me, `GetPersistentSetting()` gets values from `bitcoin.conf`, so if you set `upnp=1` in bitcoin.conf, the branch below is executed, even if there's no upnp setting in `settings.json`. Maybe this is intended, but in that case `if (value.isTrue)` below fails, so natpmp does not get enabled, despite the warning that gets logged.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1964369624)
fixed, thanks

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1964369764)
fixed

💬 davidgumberg commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#issuecomment-2672735504)
Concept ACK

I agree with the motivation that most users don't "care about what method of automatic port forwarding method is used." (https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671668235)

I think it is a little bit unfortunate that there might be some users of the GUI who don't check the release notes or debug.log whose networks did support UPnP but don't support NAT-PMP or PCP will never be warned that their port forwarding setup is broken, but I think the negatives of
...

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-2672741560)
Forced pushed https://github.com/bitcoin/bitcoin/compare/46f0bc5b485130d3e7d14e6855d81d387950cb39..6dec2d4fed3c6ca8ebef05988fded44ad9051a77 to fix @glozow comments

💬 ryanofsky commented on pull request "build: create Depends build type for depends and use it by default for depends builds":
(https://github.com/bitcoin/bitcoin/pull/31920#issuecomment-2672745734)
Current integration between depends and cmake builds definitely does not seem ideal, but it's hard for me to figure out if whether this PR makes the situation better. Maybe the PR description can be updated to state some advantages of this new approach? Or some problems with the current approach that this new approach solves?

I listed a few aspects of the cmake/depends integration that I thought seemed broken and counterintuitive in https://github.com/bitcoin/bitcoin/issues/30813#issuecommen
...

💬 polespinasa commented on pull request "wallet, rpc: deprecate settxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2672793408)
Thanks for reviewing. I agree, startup option should be also deprecated.
Will work on that.

💬 Pumpkin1234567812 commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2672800460)
ApplyDependencies(0);

📝 fanquake converted_to_draft a pull request: "Execute Discover() when bind=0.0.0.0 or :: is set"
(https://github.com/bitcoin/bitcoin/pull/31492)
If `-bind=0.0.0.0` or `-bind=::` is specified, it indicates that we can bind to any available address. In this case, the `Discover()` function should be executed to retrieve the local addresses. This behavior should also work for "=onion" addresses.

I updated the `GetListenPort()` function to also consider ports from onion address. Without this change, when `-bind=0.0.0.0:port=onion` is used, the bound port would not match the specified port.

### How to test
Prepare the addresses to be fo
...