👍 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]
🤔 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
(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.
(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
(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.
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2100477997)
No, 4MB! Great catch thanks, fixed.