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

💬 ryanofsky commented on pull request "[DRAFT] ipc: add windows support":
(https://github.com/bitcoin/bitcoin/pull/32387#issuecomment-2848129853)
Updated 27328195136754babe8d8f5d99f4f0b6a5ab2e55 -> f215e742045401ab386f0cd67d06b358558ecf89 ([`pr/ipc-win.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.4) -> [`pr/ipc-win.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-win.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-win.4..pr/ipc-win.5)) fixing many windows bugs.

With this update, IPC code is mostly working on windows: `mpexample` and `mptest` programs work, `test_bitcoin` IPC calls over socketpair
...