💬 fanquake commented on issue "guix: Build fails for `x86_64-apple-darwin`":
(https://github.com/bitcoin/bitcoin/issues/30810#issuecomment-2335396254)
You'll need to try with all the options we pass to shell/container.

💬 furszy commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1748270119)
In case we add another action in the future, this should be:
```suggestion
return action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile();
```

💬 furszy commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1748327016)
> Setting a null setting should just delete a setting consistently and not have different behavior for different functions.

I would rather fail when a null setting is provided to any of the update or overwrite functions. I believe the intent to delete should be explicit, at least within this settings layer. That's why the `deleteRwSettings` function was introduced. It seems inconsistent to have two different methods for deleting a value - one embedded within the update function and another ex
...

⚠️ Graysonbarton opened an issue: "Backwards compatibility to the Genesis block."
(https://github.com/bitcoin/bitcoin/issues/30843)
https://github.com/devcontainers/templates/pull/286

✅ achow101 closed an issue: "Backwards compatibility to the Genesis block."
(https://github.com/bitcoin/bitcoin/issues/30843)

:lock: achow101 locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30843)

💬 TheCharlatan commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2335516571)
I don't really see a difference in performance to be honest.

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

Building on Windows benefits most.

🤔 zaidmstrr reviewed a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2287939990)
ACK [590d2fb](https://github.com/bitcoin/bitcoin/pull/30679/commits/590d2fb0d8548d6d55fd1c738c8bcbe71a57aaf2)
I have reviewed and tested by manual and dry running the code to detect any edge cases and coding related erros. I also performed manual fuzzing by passing arguments in the CLI using bitcoind to check for any unexpected behaviours and to check whether the new code giving the updated results without any bypass or failure. 

📝 furszy opened a pull request: "RPC: improve SFFO arg parsing, error catching and coverage"
(https://github.com/bitcoin/bitcoin/pull/30844)
Following changes were made:

1) Catch and signal error for duplicate string destinations.
2) Catch and signal error for invalid value type.
3) Catch and signal error for string destination not found in tx outputs.
4) Improved `InterpretSubtractFeeFromOutputInstructions()` code organization.
5) Added test coverage for all possible error failures.

Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
- PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class decl
...

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