Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 laanwj commented on pull request "node: cap `-maxmempool` and `-dbcache` values for 32-bit":
(https://github.com/bitcoin/bitcoin/pull/32530#issuecomment-2890261145)
i wonder how many people are running bitcoin nodes on 32-bit systems these days. Is this still worth special casing?
💬 Sjors commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-2890264852)
utACK d316a75f1fb807f5384ec9d4ed0b8aad6df7ed5f

This causes the version to be missing from some of the "Please report this issue here" messages. But if someone is running a node, rather than some kernel based application, they can easily find its version when reporting the bug.
💬 laanwj commented on pull request "RFC: bench: replace embedded raw block with configurable block generator":
(https://github.com/bitcoin/bitcoin/pull/32554#issuecomment-2890272937)
Concept ACK
💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258063)
It exercises both `GetSigOpCount` and `GetLegacySigOpCount`, so I kept it.
But the new tests can indeed be renamed, thanks!
💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258155)
Indeed, added, thanks.
💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258344)
I have tried including `interpreter.h` before, but it results in ugly weird compiler mess.
But it seems we can move those constant over to `script.h` instead - done, added you as a co-author.
💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258456)
I took these definitions from `solver.cpp` - the point here is not necessarily to fully validate the internal structure of the script templates, but rather to check if it follows a general template's layout (opcode, followed by a version, followed by a hash, etc). Validating the existing versions is done in other places, it's not important for e.g. SigOp count. If you think I'm mistaken here, please let me know.
💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2095258535)
This wasn't checked in `MatchPayToPubkey` either.
💬 l0rinc commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2890294507)
Thanks for the observations @tapcrafter, addressed your concerns in the latest push, adding you as a co-author in one of the commits.

@maflcko, to clarify the effect on the overall behavior, I slimmed down the `DeserializeAndCheckBlockTest` similarly to https://github.com/bitcoin/bitcoin/pull/32457 and added the results to the PR's description.

---

I ran

```bash
cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release
cmake --build build -j$(nproc)
build/bin/bench_bitcoin -filte
...
🚀 fanquake merged a pull request: "scripted-diff: test: remove 'descriptors=True' argument for `createwallet` calls"
(https://github.com/bitcoin/bitcoin/pull/32544)
💬 maflcko commented on pull request "RFC: script: short-circuit `GetLegacySigOpCount` for known scripts":
(https://github.com/bitcoin/bitcoin/pull/32532#issuecomment-2890357028)
Ah, ok. So from extrapolating the previous comment and https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2606765689, the IBD speedup should be around 1%?
📝 willcl-ark opened a pull request: "doc: add alpine build instructions"
(https://github.com/bitcoin/bitcoin/pull/32559)
Closes: #32512

- Provide Alpine build instructions in build-unix.md.
- Instructions cover building all components except USDT, which requires some `libc`-specific functionality.

Marked as draft as I have not tested the build GUI under a real graphical environment yet. We could alternatively omit support for the GUI on Alpine.
💬 maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout)":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890411766)
So far it looks like this can only happen with gcc-13 (or older) *and* GLIBCXX debug mode. I don't think any real user is running in production with the std lib debug mode, so an alternative would be to just fixup the CI?

For reference, the end-user setting `-D_GLIBCXX_ASSERTIONS` passed 25 times: https://cirrus-ci.com/task/5739615073599488
💬 laanwj commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout)":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890480867)
> so an alternative would be to just fixup the CI?

Sure. If this is unlikely to cause issues for real users, that makes more sense, the point of the CI is not to keep us patching the code to make the CI happy 😄
📝 maflcko opened a pull request: "ci: Move DEBUG=1 to centos task"
(https://github.com/bitcoin/bitcoin/pull/32560)
The glibcxx debug mode has many bugs in prior gcc releases:

* https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890411766
* https://github.com/bitcoin/bitcoin/issues/31436#issuecomment-2530717875
* ...

Instead of working around all of them, just use the existing `ci_native_centos` task with gcc-14 to have it enabled. This also follows the logic of other sanitizers (tsan, asan, ubsan, msan, valgrind, ...) to generally prefer the latest version of the sanitizer for the latests
...
💬 fanquake commented on issue "intermittent issue in qt addressBookTests: (table_view->model()->rowCount()): [3 != 2] Loc: [./qt/test/addressbooktests.cpp(183)]":
(https://github.com/bitcoin/bitcoin/issues/32558#issuecomment-2890508864)
https://github.com/bitcoin/bitcoin/runs/42411027184
📝 maflcko opened a pull request: "doc: Adjust stale MSVC bug url"
(https://github.com/bitcoin/bitcoin/pull/32561)
The old url is stale, so use the current one. See https://github.com/bitcoin/bitcoin/pull/32552#issuecomment-2889188342
💬 maflcko commented on pull request "refactor: Remove workaround for resolved MSVC bug":
(https://github.com/bitcoin/bitcoin/pull/32552#issuecomment-2890510445)
> > It would be good to adjust the URL to avoid confusion and duplicate work and pull requests
>
> Sure. I'll wait until it's been properly triaged.

See https://github.com/bitcoin/bitcoin/pull/32561