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

💬 fanquake commented on pull request "Execute Discover() when bind=0.0.0.0 or :: is set":
(https://github.com/bitcoin/bitcoin/pull/31492#issuecomment-2672810727)
> I will fix the merge conflicts.

Moved to draft. @andremralves are you circling back to fix the conflicts?

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2672823436)
Rebased due to the conflict with the merged bitcoin/bitcoin#31742.

⚠️ maflcko opened an issue: "intermittent ipc_tests (Timeout)"
(https://github.com/bitcoin/bitcoin/issues/31921)
https://cirrus-ci.com/task/4822889158934528?logs=ci#L4124

```
test/ipc_tests.cpp(11): Entering test suite "ipc_tests"
test/ipc_tests.cpp(12): Entering test case "ipc_tests"
2025-02-20T19:40:40.807963Z [unknown] [test/util/random.cpp:48] [SeedRandomStateForTest] Setting random seed for current tests to RANDOM_CTX_SEED=d9d17e81b3c451b4796c66be71a75826f4089e0ed585b15f9fe9784685ea3f80
2025-02-20T19:40:40.809986Z [test] [init/common.cpp:151] [LogPackageVersion] Bitcoin Core version v28.99.0-46a9c730
...

🚀 hebasto merged a pull request: "qt: Update `src/qt/locale/bitcoin_en.xlf` after string freeze"
(https://github.com/bitcoin-core/gui/pull/854)