💬 ryanofsky commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1748697758)
So we both agree it is bad for overwrite function to reject null values and update function to accept them, but I would prefer both functions to accept them and you want both functions to reject them.

Here are the reason I think it is better to accept them:

- I think it is generally bad practice to accept nullable values and then handle them with runtime errors. If you want to write API's that don't accept null values I think you should use non-nullable types.

- I think in this specific
...

🤔 tdb3 reviewed a pull request: "docs: Updated debug build instructions for cmake"
(https://github.com/bitcoin/bitcoin/pull/30840#pullrequestreview-2288114284)
Concept ACK

Looks like the Win64 CI error is unrelated.

📝 hebasto opened a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30845)
This PR updates the libsecp256k1 subtree to https://github.com/bitcoin-core/secp256k1/commit/2f2ccc469540fde6495959cec061e95aab033148, which includes the following changes:
- https://github.com/bitcoin-core/secp256k1/pull/1577
- https://github.com/bitcoin-core/secp256k1/pull/1578
- https://github.com/bitcoin-core/secp256k1/pull/1583
- https://github.com/bitcoin-core/secp256k1/pull/1586
- https://github.com/bitcoin-core/secp256k1/pull/1600

The latter is required for https://github.com/bit
...

💬 hebasto commented on pull request "build: Use correct variable name":
(https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2336117063)
Rebased on top of the https://github.com/bitcoin/bitcoin/pull/30845.

👍 tdb3 approved a pull request: "test: Add coverage for dumptxoutset failure robustness"
(https://github.com/bitcoin/bitcoin/pull/30817#pullrequestreview-2288181583)
ACK c2b779da4e7f1bf1a5c5d67ec94cba3027b42ee7

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2336263097)
Not sure if the previous releases CI failure (timeout) is unrelated (https://cirrus-ci.com/task/6175438823751680). Nothing sticks out immediately to me, except for possibly the following (but maybe this is just a symptom?):
```
+ podman container rm --force --all
time="2024-09-07T09:55:54Z" level=warning msg="StopSignal SIGTERM failed to stop container ci_native_previous_releases in 10 seconds, resorting to SIGKILL"
ef7ef7197494aa565138bc0560b4937dd11a79ade74357aa89bbf111955959e0
```

Al
...

👍 tdb3 approved a pull request: "Drop -dbcache limit"
(https://github.com/bitcoin/bitcoin/pull/28358#pullrequestreview-2288330073)
ACK bb3b980dfd96d9a89e6b04aef2859ce0555c6807

Seems reasonable that a user explicitly setting `dbcache` would have the freedom (and responsibility) to allocate as much memory as desirable.

Rebased this PR branch on top of master (d661e2b1b771abafb0b152842d775d3150032230).
```
2024-09-06T03:37:15Z Cache configuration:
2024-09-06T03:37:15Z * Using 2.0 MiB for block index database
2024-09-06T03:37:15Z * Using 8.0 MiB for chain state database
2024-09-06T03:37:15Z * Using 42998.0 MiB for in
...

💬 naiyoma commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#issuecomment-2336408991)
Concept ACK
+1 on the suggestion above to move `BITCOIN_PID_FILENAME_DEFAULT` and import it in both files.

This is out of scope for this PR, so feel free to ignore.
it would be good to add an edge case to test starting a node when there is already a pre-existing PID file.

💬 hebasto commented on issue "guix: Build fails for `x86_64-apple-darwin`":
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2336415272)
The problem was `env SDK_PATH=/home/hebasto`, which effectively shared my home directory with a Guix container.

✅ hebasto closed an issue: "guix: Build fails for `x86_64-apple-darwin`"
(https://github.com/bitcoin/bitcoin/issues/30810)

💬 hodlinator commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1748936794)
The error looks very much like what I ran into in my attempt to make `CScript` take a `span`, see my change to `prevector` in 05c16ae8649ee3b6bbcdbdf0541a65ccf4477537 (referenced in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715642251), I believe that might surprisingly enough fix the issue.

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1748938887)
Wow, what a weird fix!
Thanks a lot, added it in a separate commit, let's see if this fixes it indeed: [`18870ac` (#30765)](https://github.com/bitcoin/bitcoin/pull/30765/commits/18870ac2eb419fcadd3f055c373f464a7319482e)

👍 hodlinator approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2288367006)
re-ACK 43cd83b0c7ba436b8ffc83d8a65e935714dffe70

`git range-diff 43cd83b~5 b60013 43cd83b`

- Added back slightly useless "Construct from hex string" section to make the PR more straight-forward to review (and commit history arguably cleaner as a consequence).
- Added half of my suggestion for bringing back testing against integer literals in `conversion` test. :+1:

(Passed `ctest` locally).

Agree with l0rinc's nits though (https://github.com/bitcoin/bitcoin/pull/30773#discussion_r174
...

💬 davidgumberg commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2336493024)
> @sipa No, it's exactly the same with `-blocksxor=0`.

This has uncovered a bug in #28052, `InitBlocksdirXorKey` does not return an empty `std::vector<std::byte>` when the `-blocksxor=0` argument is used, it instead [returns](https://github.com/bitcoin/bitcoin/blob/fa460884406b35b0dee75af23f42e8b4c4acbebc/src/node/blockstorage.cpp#L1185) a [vector with 8 bytes equal to zero](https://github.com/bitcoin/bitcoin/blob/fa460884406b35b0dee75af23f42e8b4c4acbebc/src/node/blockstorage.cpp#L1152):

`
...

🤔 tdb3 reviewed a pull request: "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD"
(https://github.com/bitcoin/bitcoin/pull/30807#pullrequestreview-2288434916)
Concept ACK

Brief code review.
Looks good. Planning to review in more detail.

💬 BenWestgate commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2336527820)
> ### `dbcache=43008`
> Time to reach block 860036: 6 hours 42 minutes Time to exit bitcoind (writing out): 110 minutes (cache was 26691.3MiB at the time) Significant write out seen only when exiting bitcoins.

The long writing out would go significantly faster on a higher performance SSD, it is also CPU-intensive as it's compressing the 27GB in memory. So the faster initial sync is a win.

💬 ajtowns commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2336545633)
> Motivation is still the same to limit the DoS exposure in face of some DoSy behaviors from tx-relay peers.

If this is just something a peer can opt-in to, I don't think it mitigates any DoS behaviours: spammers/attackers will just use the old version.

I'm pretty skeptical that anything here can really prevent *attacks* -- an attacker can run multiple or low-latency connections to end up with pretty much the same result in most cases, I think. As far as I can see the main benefit is to ma
...

👋 hebasto's pull request is ready for review: "ci: Post CMake-migration fixes and amendments"
(https://github.com/bitcoin/bitcoin/pull/30841)

📝 hebasto opened a pull request: "test: Drop no longer needed workarounds"
(https://github.com/bitcoin/bitcoin/pull/30847)
This PR deletes the workarounds introduced in https://github.com/bitcoin/bitcoin/pull/16564 and https://github.com/bitcoin/bitcoin/pull/15382, as `ctest` skips these cases gracefully: https://github.com/bitcoin/bitcoin/blob/5c80192ff6b982ee3a75be4142fe942b8206f2cd/src/test/CMakeLists.txt#L201-L203

💬 hebasto commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2336612503)
> > I am not measuring a difference in performance in the generation of the headers between master and this PR.
>
> Building on Windows benefits most.

On my Windows machine, the time to process `data/*.h` headers for the `test_bitcoin` target has been reduced from ~155s to ~105s.

@sipsorcery What do you think?

💬 laanwj commented on pull request "security-check: test for `_FORTIFY_SOURCE` usage in release binaries":
(https://github.com/bitcoin/bitcoin/pull/27038#issuecomment-2336616645)
ACK 6c9000cfbfab1cd3b48efebd8e0e90ae597cf561

Building on x86_64 gets different guix output for MacOS. But this is unrelated to this PR.
```
-129b365bba906f50926218d5b8bb76921f5f637549fe44941e01eb8ba5d5d5f0 guix-build-6c9000cfbfab/output/arm64-apple-darwin/bitcoin-6c9000cfbfab-arm64-apple-darwin-unsigned.zip
+f30b60f67b6415c6598a64aeb64f5653349e9dfdeacb901c4173a73fb725904f guix-build-6c9000cfbfab/output/arm64-apple-darwin/bitcoin-6c9000cfbfab-arm64-apple-darwin-unsigned.zip
-f836231d54d1
...