Bitcoin Core Github
44 subscribers
121K links
Download Telegram
🤔 hodlinator reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2517170254)
Code reviewed 80608ba5d282ae8713ea0136ea9a0208b254c082

`git range-diff master 54ad580 80608ba`
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893895081)
Typo:
```
`-norpcwallet`, and `-no have
```

Also not sure this should be included in this sentence assuming it is `-asmap` since it was not used as a list arg?
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893738529)
> Reason for doing this is to fit in with existing tests in this file, and to fail right away instead of timing out when the expected log prints are missing. Tests that time out instead of failing are unpleasant to work with.

The rest of the tests are not using the new `info` functionality, so my suggestion is arguably a better fit, and your version would still time out if `addcon_thread_started` was not found (although that timeout would happen in earlier checks). Not sure we should be optim
...
💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1893728563)
Should people really be running functional tests with those flags?
```
~/bitcoin/test/functional
₿ git grep -E "\bassert\b" |wc -l
1499

~/bitcoin/test/functional
₿ git grep assert |wc -l
7228
```
My bet is that more than a handful of those 20.7% cases are not for internal assumptions.

If we really want to only use `assert` for internal assumptions maybe we shouldn't be using `AssertionError` for other cases and should not be calling our custom functions `assert_*`?

Tried running
...
💬 hodlinator commented on pull request "fuzz: Faster leak check when generating corpus":
(https://github.com/bitcoin/bitcoin/pull/31481#issuecomment-2556961836)
> Just for completeness, the performance increase should be mild overall, assuming that the leak detection is turned off automatically anyway for long runs, but I haven't confirmed this.
>
> If [#31481 (comment)](https://github.com/bitcoin/bitcoin/pull/31481#discussion_r1882498320) is too controversial, an alternative could be to just disable it when `empty_min_time` is active, or just close this pull.

Yeah, if libfuzzer automatically stops leak detection after a while, I can see a case fo
...
👍 hebasto approved a pull request: "test: Embed univalue json tests in binary"
(https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517520234)
re-ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b.
🤔 maflcko reviewed a pull request: "build: enable libc++ and libstdc++ hardening"
(https://github.com/bitcoin/bitcoin/pull/31424#pullrequestreview-2517521376)
concept ack, but it would be good to get the (CI) changes right
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2556981982)
(assigned milestone, because this is a blocker to run the tests on native windows, which would be nice to *finally* get running)
⚠️ hebasto opened an issue: "qa: `AssertionError: not(10.00000000 == 340)` in `wallet_assumeutxo.py --descriptors`"
(https://github.com/bitcoin/bitcoin/issues/31546)
https://github.com/hebasto/bitcoin/actions/runs/12431224655/job/34708713770:
```
265/315 - wallet_assumeutxo.py --descriptors failed, Duration: 5 s

stdout:
2024-12-20T12:54:28.609000Z TestFramework (INFO): PRNG seed is: 187204816715607970

2024-12-20T12:54:28.641000Z TestFramework (INFO): Initializing test directory D:\a\_temp\test_runner_₿_🏃_20241220_123237\wallet_assumeutxo_48

2024-12-20T12:54:30.237000Z TestFramework (INFO): -- Testing assumeutxo

2024-12-20T12:54:30.237000Z Tes
...
📝 l0rinc opened a pull request: "build: Use character literals for generated headers to avoid narrowing"
(https://github.com/bitcoin/bitcoin/pull/31547)
Use character literals instead of integer hex values (i.e. `'\x5b','\x0a', ...` instead of `0x5b, 0x0a, ...`) for generated headers. This avoids C++11 narrowing warnings in a more concise way than using explicit char casts.

Extra whitespace is also removed between elements for brevity.

Split out of https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893781576
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893953186)
Extracted to https://github.com/bitcoin/bitcoin/pull/31547 - pease resolve the comment
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2557043408)
ACK faedcc5fdabc508aaade9b461e44ea3a6670f36b

Avoiding file reads during test setup is a step forward in portability 👍
We can continue the refactors in follow-up PRs.
👍 theStack approved a pull request: "doc: Install `py3-zmq` port on OpenBSD for `interface_zmq.py`"
(https://github.com/bitcoin/bitcoin/pull/31535#pullrequestreview-2517585410)
Tested ACK be1a2e5dfbdf4b3799f07e48c41b09b54b72f9b6

Verified on OpenBSD 7.6 that `$ ./build/test/functional/interface_zmq.py` succeeds if `py3-zmq` is installed and that the test is skipped (`python3-zmq module not available`) if that package is removed.
💬 maflcko commented on issue "qa: `AssertionError: not(10.00000000 == 340)` in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/31546#issuecomment-2557052983)
Is it reproducible?
💬 maflcko commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#discussion_r1893980063)
Thx, rebased to avoid changing the same line twice.
💬 l0rinc commented on pull request "test: Embed univalue json tests in binary":
(https://github.com/bitcoin/bitcoin/pull/31542#issuecomment-2557102250)
ACK faf7eac364fb7f421a649b483286ac8681d92b31

The changes since last ACK: [simplify char casting](https://github.com/bitcoin/bitcoin/pull/31547) and reformat the affected sources.
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2557107378)
`f5fc9451aa...3522e897f2`: rebase due to conflicts
🚀 fanquake merged a pull request: "cmake: Remove unused `BUILD_TESTING` variable from "dev-mode" preset"
(https://github.com/bitcoin/bitcoin/pull/31544)
🚀 fanquake merged a pull request: "doc: Install `py3-zmq` port on OpenBSD for `interface_zmq.py`"
(https://github.com/bitcoin/bitcoin/pull/31535)
🚀 fanquake merged a pull request: "guix: latest 2.31 glibc"
(https://github.com/bitcoin/bitcoin/pull/31529)