💬 maflcko commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2100435827)
> Tinyformat also uses `%s` for strings without `c_str()`, which isn't explicitly documented.

It *is* documented in this file ;)



> Well the idea was that by the time the compiler gets to it, a developer may already have done a frustrating search "what % expression do i need to use". Documenting it could avoid that

Ok, restored and clarified it for now. It will probably go away in 3 years with C++23 std::format

👍 fanquake approved a pull request: "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`"
(https://github.com/bitcoin/bitcoin/pull/32551#pullrequestreview-2857948411)
ACK 800b7cc42ca63f2a6b245a4d327c7092289da6e1

💬 maflcko commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2100437912)
> maybe suggest either...

thx, done

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100440303)
That should work but the reason I didn't leave like that is I don't feel like its effectively testing the right thing. We could start the timer at the top of the file and it would always pass, but it wouldn't necessarily catch any regressions. The server timeout should re-start every time it receives a packet from the client.

Another approach I tried was actually setting libevent debug logs and trying to track down messages like these:

```
event_add: event: 0x10a004210 (fd 20), EV_READ
...

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100443031)
All that being said, if you want me to use the patch you wrote that's fine by me too ;-)

💬 maflcko commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2100449671)
> Small suggestion also add in CONTRIBUTING to wait for CI to pass.
>
> Lines 136-137:
>
> * Push changes to your fork
>
> * Wait for the CI to pass
>
> * Create pull request

I don't think this works. Contributors may have to enable GHA, which they may not want. Also, the Cirrus tasks are tedious to setup and require hardware/vm runners. Also, some tasks do different things in a pull request vs branch. Finally, it seems overkill to run (and wait for) CI twice.

Most o
...

💬 maflcko commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#issuecomment-2898187007)
> I see the benefit of not duplicating linter code in developer notes. However it's quite annoying to find out about linter failures only after pushing.
>
> The developer notes could point to the incantation to run all the linters in a container.

The lint CI should always run and complete within 60 seconds after pushing. If not, please let me know.

Your note makes sense. I'll consider it in the next doc update, which moves stuff around.

💬 maflcko commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2898213655)
> so it might result in a hidden performance regression instead of just a compile error.

I'd guess on modern, fast storage this doesn't make a visible difference. (Would be good to double check)

Then, given that the fallback seems to be needed by OpenBSD and newlibc (possibly others), it may be better to keep it?

Ref: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/15163923250/job/42638498492?pr=54#step:6:361 (OpenBSD 7.7):

```
src/util/fs_helpers.cpp:204:14: error: use
...

🤔 maflcko reviewed a pull request: "Fees: add Fee rate Forecaster Manager"
(https://github.com/bitcoin/bitcoin/pull/31664#pullrequestreview-2858027514)
(one more lint, also the ci test-each-commit failed)

💬 maflcko commented on pull request "Fees: add Fee rate Forecaster Manager":
(https://github.com/bitcoin/bitcoin/pull/31664#discussion_r2100486155)

CachedMempoolForecast holds a cache of recent fee rate forecast. -> CachedMempoolForecast holds a cache of recent fee rate forecasts. [grammatical error: "forecast" should be plural when referring to multiple items]

🤔 pinheadmz reviewed a pull request: "tests: Expand HTTP coverage to assert libevent behavior"
(https://github.com/bitcoin/bitcoin/pull/32408#pullrequestreview-2858010430)
rebase to f16c8c67bf to address comments, not touching the `rpcservertimeout` test just yet, discussion still ongoing

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100474952)
Ok I'll change the value here to 5 -- meaning the `sock.recv()` on L139 will always take that much time. That will, however, protect us against false positives on a slow CI, if the server regressed and actually responded to the requests out of order, but took more than 1 second to do so.

After we generate a block, the `sock.recv()` on L150 should execute very quickly, but will allow up to 5 seconds for a slow CI to respond to the two RPC requests. That applies to the `sendall()` as well.

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100476656)
thanks, done

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100477997)
No, 4MB! Great catch thanks, fixed.

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100481900)
awesome yes, thanks, done

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100488438)
yep thanks

📝 rkrux opened a pull request: "wallet, test: best block locator matches scan state follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32580)
Few follows-ups from #30221: Use `SetLastBlockProcessedInMem` more in `AttachChain`, add not null locator check in `WriteBestBlock`. Add log and few assertions in `wallet_reorgstore` test.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide cl
...

💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#discussion_r2100509926)
What would you suggest as a test case?

💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2898284120)
Reverted to the prior change, given we cannot assume availablility.

💬 ryanofsky commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898288496)
I'd think for both windows and unix, a good approach could be not parse -signer strings, just leave it up to the shell on unix, and shell+appilcation itself on windows (because on windows the application runtime is responsible for splitting arguments, usually using [CommandLineToArgv](https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw)) function. (This was also discussed pretty extensively in https://github.com/bitcoin/bitcoin/pull/13339)

If we can avo
...