⚠️ John-zhan opened an issue: "depends config fails"
(https://github.com/bitcoin/bitcoin/issues/32578)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
"cmake -B build --preset vs2022-static" execute this config, and bitcoin--9.0\build\vcpkg_installed\vcpkg\blds\libevent\config-x64-windows-static-out.log display such info:
[1/2] "D:/Program Files/CMake/bin/cmake.exe" -E chdir ".." "D:/Program Files/CMake/bin/cmake.exe" "F:/workspace/code/github/bitcoin-29.0/build/vcpkg_installed/vcpkg/blds/libevent/src/.12-stable-1d4b950f74.clean" "-G" "
...
(https://github.com/bitcoin/bitcoin/issues/32578)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
"cmake -B build --preset vs2022-static" execute this config, and bitcoin--9.0\build\vcpkg_installed\vcpkg\blds\libevent\config-x64-windows-static-out.log display such info:
[1/2] "D:/Program Files/CMake/bin/cmake.exe" -E chdir ".." "D:/Program Files/CMake/bin/cmake.exe" "F:/workspace/code/github/bitcoin-29.0/build/vcpkg_installed/vcpkg/blds/libevent/src/.12-stable-1d4b950f74.clean" "-G" "
...
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2100274925)
It didn't compile - couldn't convert the return value of `snap.Nodes()` which is `const std::vector<std::shared_ptr<CNode>>&` to `std::span<std::shared_ptr<CNode>>`. So I used vector in the first commit, and later when the snapshots are removed - changed it to `span`.
(https://github.com/bitcoin/bitcoin/pull/32015#discussion_r2100274925)
It didn't compile - couldn't convert the return value of `snap.Nodes()` which is `const std::vector<std::shared_ptr<CNode>>&` to `std::span<std::shared_ptr<CNode>>`. So I used vector in the first commit, and later when the snapshots are removed - changed it to `span`.
💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2897938845)
`cbd7bd7422...67bc008f62`: fix CI
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2897938845)
`cbd7bd7422...67bc008f62`: fix CI
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100286223)
The value of a transaction output.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2100286223)
The value of a transaction output.
🚀 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.