🤔 pinheadmz reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2812313762)
code reviewed at 41728b516e

Built and tested on arm64/macos. Played with the feature locally using 10 and 30 second block times. A few comments below, my main concern being a mismatch between the python miner and the custom consensus rules. Didn't have time to try and mine 2, 016 signet blocks to check that difficulty adjusted as expected, but otherwise everything seems to work as expected.

Consensus code is touched, but only for signet ;-)

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2071792992)
> currently the only supported parameter is target spacing, the format of <params> to set it is 01<8 bytes value of target spacing, seconds, little endian>

I think you can remove this, it could get carried away if more parameters are added in the future. You could just link to verbose documentation in signet/README.md or reccomend using signet/miner, which leads me to a second suggestion:

Add a new command to signet/miner like `generate`, `solvepsbt` etc... maybe `makechallenge` that accep
...

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2071904374)
Why not pass `options` and `args` to one function `ParseSignetChallenge()` that calls the two functions you have, and fills in the `SigNetOptions` members where applicable? In other words, why write data to these vectors?

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2071915516)
I would expect to see a loop here that parses the byte vector:

1. read byte -- if its unknown (not `01`, currently) throw an InitError
2. for byte == `01` read the next 8 bytes to set the value
3. repeat for all future extensions

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2071913713)
Don't we have a serializer helper method for this kind of thing?

🤔 l0rinc requested changes to a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2812640885)
Code review fe2d1955f6333c57f54aed2a2a8b3cdaa6bf9d71

* blocker: the new `AutoFile::write_buffer` should also mark the file as dirty.
* the current `fclose` returns a fake error code, but is actually a boolean - may make sense to rename it (e.g. `Close`, similarly to how we've wrapped `ftruncate` with `TruncateFile`) and migrate to returning a boolean to avoid the current confusion.

<details>
<summary>Diff since last review</summary>

```patch
diff --git a/src/index/blockfilterindex.cp
...

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071988064)
> because here from the destructor

Still not sure what this means exactly, would this maybe work better?

```suggestion
// This is because here in the destructor we have no way to signal
// errors from close(), which, after write, could mean the file is
```

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2072005944)
do we need both messages? How will this be displayed in the logs, isn't it redundant?

💬 hebasto commented on issue "ctest -C Debug fails on vs2022 (miniscript_tests (SEGFAULT))":
(https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2847867432)
> > A third question would be, why the fuzz output is https://github.com/hebasto/bitcoin/actions/runs/14650416249/job/41114604643#step:14:60: `subprocess timed out: Currently only libFuzzer is supported`. A timeout should not happen for this process call, and the expected outcome should be an immediate stderr of `test/fuzz/fuzz.cpp:269 main: Assertion 'read_file(input_path, buffer)' failed. Error processing input "-help=1"` and an ignored exit code.
>
> Looks like the assertions fails, but the
...

📝 pinheadmz opened a pull request: "tests: Expand HTTP coverage to assert libevent behavior"
(https://github.com/bitcoin/bitcoin/pull/32408)
These commits are cherry-picked from #32061 and part of a project to [remove libevent](https://github.com/bitcoin/bitcoin/issues/31194).

This PR only adds functional tests to `interface_http` to cover some HTTP server behaviors we inherit from libevent, in order to maintain those behaviors when we replace libevent with our own HTTP server.

1. Pipelining: The server must respond to requests from a client in the order in which they were received [RFC 7230 6.3.2](https://www.rfc-editor.org/r
...

💬 pinheadmz commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2847932450)
Crashed with long time spacing (are you sure you need 2^64 seconds?)

`build/bin/bitcoind -signet -signetchallenge=6a4c09011effff00000000004c0151 -debug -debugexclude=libevent`

`bitcoin-cli -signet -generate`

log:
```

2025-05-02T19:18:23Z [http] Received a POST request for / from 127.0.0.1:39282
2025-05-02T19:18:23Z [rpc] ThreadRPCServer method=getnewaddress user=__cookie__
2025-05-02T19:18:24Z [http] Received a POST request for / from 127.0.0.1:39284
2025-05-02T19:18:24Z [rpc] Th
...

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2847960905)
Concept ACK

💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2847976226)
Also added a commmit to fix the too-large args (https://github.com/bitcoin/bitcoin/pull/32399#issuecomment-2846837427).

📝 hebasto opened a pull request: "fuzz: Suppress abort message on Windows"
(https://github.com/bitcoin/bitcoin/pull/32409)
This PR ensures that the `fuzz.exe` binary is non-interactive, which is essential for CI tests.

Here are CI jobs for the "Debug" configuration:
- on the master branch: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/14759346479/job/41435857332
- with this PR: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/14801894543/job/41562322239

Address this [comment](https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2842716175).

💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r2072086633)
As the `is_eff_value` arg is always true for all the tests. What about removing it?.
Also, you are expecting `group.GetSelectionAmount()` to be equal to `tx.vout[0].nValue` with this parameter right?. I'm unsure that will always be the case.
As a test, you could try adding an assertion at the end of this function. I have a hunch it will crash.

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2848017666)
My Guix build:
```
50de79d47cf8e15acd94d189a88423a89003e287c0e30978d45cb13a478a8a7c guix-build-665b2ba99955/output/dist-archive/bitcoin-665b2ba99955.tar.gz
edae04ce1b66195e3afb9fcdd399719e20085c0d42e55f30a17fd8d1b972b798 guix-build-665b2ba99955/output/x86_64-w64-mingw32/SHA256SUMS.part
33dfd8539f6dae306a7ee3f8ff2c6e4eafe8d7740a76629767fb4f4601c9c61c guix-build-665b2ba99955/output/x86_64-w64-mingw32/bitcoin-665b2ba99955-win64-codesigning.tar.gz
9acd0edc19836d7aeed0e776d69795b8d27f78df3c12
...

💬 achow101 commented on pull request "test: remove Boost SIGCHLD workaround.":
(https://github.com/bitcoin/bitcoin/pull/32403#issuecomment-2848082546)
ACK 3add6ab9adcd722d57c6d488581358ae9b377f1a

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2848103940)
> Why is there no sign of periodic flushing during reindex?

Indeed, it seems we do not get to [this line](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L3595) during reindex-chainstate. Will have to investigate. Periodically flushing during reindex-chainstate would be useful.

> Why did the reported utxo size increase during IBD after the change?

There is less opportunity for utxo cut-through after this change. So, some utxos that would previously have been deleted ar
...

💬 mzumsande commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2848120687)
> Indeed, it seems we do not get to [this line](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L3595) during reindex-chainstate.

It makes sense: `reindex-chainstate` is nothing more than [one call](https://github.com/bitcoin/bitcoin/blob/5b8046a6e893b7fad5a93631e6d1e70db31878af/src/node/blockstorage.cpp#L1251) to `ActivateBestChain()` (which will connect all connectable blocks). Since we only flush at the end of the function, we cannot flush more than once currently.

🚀 achow101 merged a pull request: "test: remove Boost SIGCHLD workaround."
(https://github.com/bitcoin/bitcoin/pull/32403)