Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 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
💬 maflcko commented on pull request "refactor: Remove workaround for resolved MSVC bug":
(https://github.com/bitcoin/bitcoin/pull/32552#issuecomment-2890515265)
If contributors here have a microsoft account, they may want to upvote the issue report, so that microsoft is aware that there is some interest in having it fixed.
👍 theStack approved a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2850301319)
re-ACK 6ee32aaaca4a46d42594bc16479868c76cc67fd8

(as per `$ git range-diff 2be93f7b...6ee32aaa`)
laanwj closed a pull request: "subprocess: replace `fs::directory_iterator` with `readdir`"
(https://github.com/bitcoin/bitcoin/pull/32529)
💬 laanwj commented on pull request "subprocess: replace `fs::directory_iterator` with `readdir`":
(https://github.com/bitcoin/bitcoin/pull/32529#issuecomment-2890559685)
This isn't necessary if it's worked around some other way https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2890480867