💬 laanwj commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2100344020)
That's not how it's supposed to be. The default application codepage is something that the user can configure system-wide. The purpose of the applkcation manifest is to override it so it is always UTF-8 for the application. Exactly what the microsoft documentation says here: https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page
This hasn't changed with newer windows versions, as far as i know, it's just that there is no usable UTF-8 codepage in earlier ones, so
...
(https://github.com/bitcoin/bitcoin/pull/32380#discussion_r2100344020)
That's not how it's supposed to be. The default application codepage is something that the user can configure system-wide. The purpose of the applkcation manifest is to override it so it is always UTF-8 for the application. Exactly what the microsoft documentation says here: https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page
This hasn't changed with newer windows versions, as far as i know, it's just that there is no usable UTF-8 codepage in earlier ones, so
...
📝 hodlinator opened a pull request: "headerssync: Keep tests ahead of increasing params"
(https://github.com/bitcoin/bitcoin/pull/32579)
### Motivation
Each release we increase the values of the Headers Sync constants `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` as per *doc/release-process.md*. In the next (v30) or following release, it is very likely that `REDOWNLOAD_BUFFER_SIZE` (`14827` as of v29) will exceed the `target_blocks` value used to test Headers Sync (`15000`).
This would result in the `HeadersSyncState::m_redownloaded_headers`-buffer not reaching the `REDOWNLOAD_BUFFER_SIZE`-threshold during the *sr
...
(https://github.com/bitcoin/bitcoin/pull/32579)
### Motivation
Each release we increase the values of the Headers Sync constants `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` as per *doc/release-process.md*. In the next (v30) or following release, it is very likely that `REDOWNLOAD_BUFFER_SIZE` (`14827` as of v29) will exceed the `target_blocks` value used to test Headers Sync (`15000`).
This would result in the `HeadersSyncState::m_redownloaded_headers`-buffer not reaching the `REDOWNLOAD_BUFFER_SIZE`-threshold during the *sr
...
💬 jonatack commented on issue "test: `tool_wallet.py` references no-longer used CI":
(https://github.com/bitcoin/bitcoin/issues/32576#issuecomment-2898045057)
https://github.com/bitcoin/bitcoin/pull/28116 addresses it. Can update it.
(https://github.com/bitcoin/bitcoin/issues/32576#issuecomment-2898045057)
https://github.com/bitcoin/bitcoin/pull/28116 addresses it. Can update it.
💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#issuecomment-2898062472)
> It's not clear to me how the other commits are related to the PR description though
They were addressing the review comments I added to the mentioned PR, unifying naming (e.g. `fCacheCritical` vs `should_write`), extracting boolean to named symbol (`bool first_flush{m_next_write == NodeClock::time_point::max()`), adding helper for interval random generation. Not sure what the exact objections were, but I've removed them to keep only the logging now.
(https://github.com/bitcoin/bitcoin/pull/32404#issuecomment-2898062472)
> It's not clear to me how the other commits are related to the PR description though
They were addressing the review comments I added to the mentioned PR, unifying naming (e.g. `fCacheCritical` vs `should_write`), extracting boolean to named symbol (`bool first_flush{m_next_write == NodeClock::time_point::max()`), adding helper for interval random generation. Not sure what the exact objections were, but I've removed them to keep only the logging now.
👍 pinheadmz approved a pull request: "test: allow all functional tests to be run or skipped with --usecli"
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2857855826)
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
Trivial changes since last ack: rebase on master, adjust for removed legacy wallet
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgt3BwACgkQ5+KYS2KJ
yTr+QxAAmhg4rpylogoG5cCmOFvE5WovZU+Q63FoYp7SRBYt9abGf/OpmrAc5jbX
1JhfEqClryvZnIZPHNUdAfQxTXfT94+5AcD02/CYUpJO1++QwuKb
...
(https://github.com/bitcoin/bitcoin/pull/32290#pullrequestreview-2857855826)
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
Trivial changes since last ack: rebase on master, adjust for removed legacy wallet
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 7c13240d7b9a03b77ad84460e80007063367e05a
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgt3BwACgkQ5+KYS2KJ
yTr+QxAAmhg4rpylogoG5cCmOFvE5WovZU+Q63FoYp7SRBYt9abGf/OpmrAc5jbX
1JhfEqClryvZnIZPHNUdAfQxTXfT94+5AcD02/CYUpJO1++QwuKb
...
💬 laanwj commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2100377809)
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 (i don't think this particular blurb is "stale" in any way besides "not changed in a long time).
That said, there are tons of examples of `strprintf` in the code, and nothing uses length modifiers. Tinyformat also uses `%s` for strings without `c_str()`, which isn't explicitly documented.
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2100377809)
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 (i don't think this particular blurb is "stale" in any way besides "not changed in a long time).
That said, there are tons of examples of `strprintf` in the code, and nothing uses length modifiers. Tinyformat also uses `%s` for strings without `c_str()`, which isn't explicitly documented.
👍 pinheadmz approved a pull request: "net: replace manual reference counting of CNode with shared_ptr"
(https://github.com/bitcoin/bitcoin/pull/32015#pullrequestreview-2857907230)
ACK 67bc008f62f9eac1616558770708c1e8547b09f0
Only rebase and trivial adjustments since last ack
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 67bc008f62f9eac1616558770708c1e8547b09f0
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgt3x8ACgkQ5+KYS2KJ
yTqEKA//W/MMHBH002NCZEngJedxatJrtDCBj/RkfA9ijfOQ+WmNjnd2QNLJa3ds
z6NlZ0zlNNh60XZHGEgdWtEk8TvPECe/E9nEFjn6pze9REcIvizn/ZzomcNWgQxy
KJAptOo63YUXaK8mSQ
...
(https://github.com/bitcoin/bitcoin/pull/32015#pullrequestreview-2857907230)
ACK 67bc008f62f9eac1616558770708c1e8547b09f0
Only rebase and trivial adjustments since last ack
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 67bc008f62f9eac1616558770708c1e8547b09f0
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmgt3x8ACgkQ5+KYS2KJ
yTqEKA//W/MMHBH002NCZEngJedxatJrtDCBj/RkfA9ijfOQ+WmNjnd2QNLJa3ds
z6NlZ0zlNNh60XZHGEgdWtEk8TvPECe/E9nEFjn6pze9REcIvizn/ZzomcNWgQxy
KJAptOo63YUXaK8mSQ
...
💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100412156)
Ah ok, any reason for the `+ i`?
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100412156)
Ah ok, any reason for the `+ i`?
💬 fanquake commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2898132974)
https://cirrus-ci.com/task/5032025251381248
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2898132974)
https://cirrus-ci.com/task/5032025251381248
💬 hebasto commented on pull request "subprocess: Handle quoted tokens properly":
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898150304)
> Alternatively, can't we just pass a vector of arguments to `RunCommandParseJSON` and avoid splitting/merging? Hmm maybe not as it comes in through one argument string `-externalsigner` anyhow, there's just some additional arguments that we add. OK this is annoying.
Exactly. We have to parse and tokenize the `-signer` argument in cases like `bitcoind -signer="/usr/bin/python3 /some_path/signer.py"`.
(https://github.com/bitcoin/bitcoin/pull/32577#issuecomment-2898150304)
> Alternatively, can't we just pass a vector of arguments to `RunCommandParseJSON` and avoid splitting/merging? Hmm maybe not as it comes in through one argument string `-externalsigner` anyhow, there's just some additional arguments that we add. OK this is annoying.
Exactly. We have to parse and tokenize the `-signer` argument in cases like `bitcoind -signer="/usr/bin/python3 /some_path/signer.py"`.
💬 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
(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
(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
(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
...
(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 ;-)
(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
...
(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.
(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
...
(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)
(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]
(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]