🚀 fanquake merged a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475)
(https://github.com/bitcoin/bitcoin/pull/32475)
💬 fanquake commented on pull request "cmake: Remove `ENABLE_{SSE41,AVX2,X86_SHANI,ARM_SHANI}` from `bitcoin-build-config.h`":
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2897963902)
Guix Build:
```bash
d899fe5104a77d8c86d40518cb872e8200142e5711689a7bec1e116fcb1807af guix-build-800b7cc42ca6/output/aarch64-linux-gnu/SHA256SUMS.part
8f81b2cd236c0116937b950964f1a4a88c0fdf1baf0d859c055153c4fceddcdc guix-build-800b7cc42ca6/output/aarch64-linux-gnu/bitcoin-800b7cc42ca6-aarch64-linux-gnu-debug.tar.gz
43812573c4d833df3cae58332dfd632afedd428fc0c46a403b872bdfb344bfb7 guix-build-800b7cc42ca6/output/aarch64-linux-gnu/bitcoin-800b7cc42ca6-aarch64-linux-gnu.tar.gz
1e93c16fa60545e3
...
(https://github.com/bitcoin/bitcoin/pull/32551#issuecomment-2897963902)
Guix Build:
```bash
d899fe5104a77d8c86d40518cb872e8200142e5711689a7bec1e116fcb1807af guix-build-800b7cc42ca6/output/aarch64-linux-gnu/SHA256SUMS.part
8f81b2cd236c0116937b950964f1a4a88c0fdf1baf0d859c055153c4fceddcdc guix-build-800b7cc42ca6/output/aarch64-linux-gnu/bitcoin-800b7cc42ca6-aarch64-linux-gnu-debug.tar.gz
43812573c4d833df3cae58332dfd632afedd428fc0c46a403b872bdfb344bfb7 guix-build-800b7cc42ca6/output/aarch64-linux-gnu/bitcoin-800b7cc42ca6-aarch64-linux-gnu.tar.gz
1e93c16fa60545e3
...
💬 laanwj commented on pull request "chainparams: Re-add seed.bitcoinstats.com":
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2897974665)
Doesn't seem this should be closed, would just be nice to have some more testing.
(https://github.com/bitcoin/bitcoin/pull/31086#issuecomment-2897974665)
Doesn't seem this should be closed, would just be nice to have some more testing.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2897976693)
`9cf56ad085...a8fcd8385e`: fix CI lint
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2897976693)
`9cf56ad085...a8fcd8385e`: fix CI lint
💬 hebasto commented on issue "depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2897987043)
@John-zhan
You could use the same workaround as we use in the CI. See https://github.com/bitcoin/bitcoin/pull/32184.
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2897987043)
@John-zhan
You could use the same workaround as we use in the CI. See https://github.com/bitcoin/bitcoin/pull/32184.
💬 l0rinc commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2898016052)
@ryanofsky, @darosior, thank you for expressing your concerns.
I have removed the optimization part, I understand why that can be considered risky. The new rebase only contains extraction of the common script parts to avoid some work duplication and to help with understanding this part of the codebase better. It's also covered extensively with deterministic and random tests, comparing against fixed values, non-standard scripts, and completely random ones compared against the original implementa
...
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2898016052)
@ryanofsky, @darosior, thank you for expressing your concerns.
I have removed the optimization part, I understand why that can be considered risky. The new rebase only contains extraction of the common script parts to avoid some work duplication and to help with understanding this part of the codebase better. It's also covered extensively with deterministic and random tests, comparing against fixed values, non-standard scripts, and completely random ones compared against the original implementa
...
👋 l0rinc's pull request is ready for review: "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`"
(https://github.com/bitcoin/bitcoin/pull/32532)
(https://github.com/bitcoin/bitcoin/pull/32532)
✅ l0rinc closed a pull request: "bench: replace benchmark block with more representative one (413567 → 784588)"
(https://github.com/bitcoin/bitcoin/pull/32457)
(https://github.com/bitcoin/bitcoin/pull/32457)
💬 l0rinc commented on pull request "bench: replace benchmark block with more representative one (413567 → 784588)":
(https://github.com/bitcoin/bitcoin/pull/32457#issuecomment-2898025622)
Closing in favor of https://github.com/bitcoin/bitcoin/pull/32554
(https://github.com/bitcoin/bitcoin/pull/32457#issuecomment-2898025622)
Closing in favor of https://github.com/bitcoin/bitcoin/pull/32554
👋 l0rinc's pull request is ready for review: "RFC: bench: replace embedded raw block with configurable block generator"
(https://github.com/bitcoin/bitcoin/pull/32554)
(https://github.com/bitcoin/bitcoin/pull/32554)
💬 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"`.