💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965620497)
Removing?

💬 marcofleon commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965630562)
Good catch. Took me a bit to see, but yeah this check should be outside of the inner `if` statement.

💬 maflcko commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965630784)
Previously the assert was exposed, now it is guarded by the same condition, making it dead code that the compiler will remove.

In other words: `{ if (a) assert(a); }` is equivalent to `{ }`.

💬 hebasto commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2674809288)
From the [OP](https://github.com/bitcoin/bitcoin/issues/31047#issue-2570089672):
> It's not clear if using our Coverage build type works with Clang, or not.

There are a few points to take into consideration:
1. Modern LLVM's coverage tool _emulates_ GCC's `gcov` version 4.2.0, which is obviously outdated.
2. Recent versions of the `lcov` tool no longer support GCC/gcov versions older than 9.x. For example:
```
lcov: WARNING: (unsupported) Function begin/end line exclusions not supported with th
...

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2674818845)
My Guix build:
```
aarch64
d94fbbb8bed0a91349c7962d108e0296d6cfef0f4438f9918026fe3e284f5a5a guix-build-568fcdddaec2/output/aarch64-linux-gnu/SHA256SUMS.part
a126b070d9baac09d151e1de17fa21e0f8283f43f0b8bce4c4d53af86f2fdc99 guix-build-568fcdddaec2/output/aarch64-linux-gnu/bitcoin-568fcdddaec2-aarch64-linux-gnu-debug.tar.gz
5a082deacad4c4d970315b6f03bbc9fe7aae2de46cc99709502838b4487d0c57 guix-build-568fcdddaec2/output/aarch64-linux-gnu/bitcoin-568fcdddaec2-aarch64-linux-gnu.tar.gz
696a670e
...

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965674009)
Ah, good catch! Gonna fix it, thanks.

💬 hebasto commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2674864859)
> So the following workflow works, and is probably what most people do: download with Safari, which automatically extracts the `gz` layer. Then double-click to extract the Tar in Finder. And then start `bitcoind` from the terminal.

I can confirm that this workflow works.

📝 hodlinator opened a pull request: "http: Make server shutdown more robust"
(https://github.com/bitcoin/bitcoin/pull/31929)
- Instead of hanging indefinitely, use timeouts and abort when HTTP connections take unreasonably long to close.
- Add request id with logging to be able to discover which one is stuck.
- Ensure we always free libevent HTTP.
- Improve documentation around libevent behavior.

Should help debug #31894.

💬 fanquake commented on pull request "http: Make server shutdown more robust":
(https://github.com/bitcoin/bitcoin/pull/31929#issuecomment-2674904296)
```bash
Assertion failed: (evb), function WriteReply, file httpserver.cpp, line 682.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/http_request/83149a7e3abd937c7bc67fa55b9a4fc9338e3d8c"

⚠️ Failure generated from target with exit code 1: ['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/src/test/fuzz/fuzz', PosixPath('/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/http_request')]
```

💬 ryanofsky commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965736047)
In commit "init: Handle dropped UPnP support more gracefully" (bd4679a608b2cbc206fa60a180b0fc4ce8a39ec4)

I think I might not understanding what this code is trying to do exactly, but it seems overcomplicated and maybe a little off. Like the check for GetSettingPath seems unnecessary, the LogWarning seems vague, the GetPersistentSetting calls not really checking for the right thing as mentioned in an earlier comment. Also the check for isTrue seems overly narrow and the UniValue(true) cast sho
...

🚀 fanquake merged a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676)

✅ fanquake closed an issue: "lint: Qt exclusions are missing files"
(https://github.com/bitcoin/bitcoin/issues/31801)

🚀 fanquake merged a pull request: "ci: Fix filtering out Qt-generated files from `compile_commands.json`"
(https://github.com/bitcoin/bitcoin/pull/31928)

🚀 fanquake merged a pull request: "contrib: update `utxo_to_sqlite` tool documentation and comment"
(https://github.com/bitcoin/bitcoin/pull/31925)

🚀 fanquake merged a pull request: "fuzz: add basic TxOrphanage::EraseForBlock cov"
(https://github.com/bitcoin/bitcoin/pull/31918)

💬 ryanofsky commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965781753)
In commit "init: Handle dropped UPnP support more gracefully" (bd4679a608b2cbc206fa60a180b0fc4ce8a39ec4)

I think it would be better to move the settings.json update code before this code doing "if upnp then softset natpmp". That way soft-setting natpmp only happens if -upnp is enabled in `bitcoin.conf` or on the command line, not if it is enabled in settings.json. So the separate cases will be handled independently and not interacting in a more complicated way.

💬 janb84 commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2674982705)
tACK,

- Ran unit test
- Ran all the tests
- Validated the code change.

To concider; change title to "must find a fitting solution" in stead of "must find a good solution", but it's a very small nit.

I also wrote a [review note](https://github.com/janb84/Bitcoin-Core_review-notes/blob/f405429fd555efc9dc7194448a08e5f4a75cf63d/reviews/PR-%2331656.md) on the PR to document my learning journey

💬 laanwj commented on pull request "init: Handle dropped UPnP support more gracefully":
(https://github.com/bitcoin/bitcoin/pull/31916#discussion_r1965808053)
> Like the check for GetSettingPath seems unnecessar

`WriteSettingsFile` will raise an exception if that check fails, it seems to make a big point of "dynamic settings being enabled".

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965817803)
Done.

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2675005276)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993