Bitcoin Core Github
44 subscribers
121K links
Download Telegram
🤔 stickies-v reviewed a pull request: "http: Use 'starts_with' for matching URI prefix"
(https://github.com/bitcoin/bitcoin/pull/30868#pullrequestreview-2300252485)
Code LGTM eb43872b84bfc4d09fe38ce0cb74722f6f9842da. Commit message and PR title need updating, this is a `tidy` change now affecting multiple modules, not an `http` change.
💬 furszy commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1756828713)
A bit delayed but here.

> So we both agree it is bad for overwrite function to reject null values and update function to accept them, but I would prefer both functions to accept them and you want both functions to reject them.
>
> Here are the reason I think it is better to accept them:
>
> * I think it is generally bad practice to accept nullable values and then handle them with runtime errors. If you want to write API's that don't accept null values I think you should use non-nullable
...
💬 Zk2u commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-2346241399)
@starius could you possibly rebase this onto latest?
💬 fanquake commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756842485)
Something like:
```bash
# Always skip the ctime tests, if we are building no other tests.
# Otherwise, they are built if Valgrind is available. See SECP256K1_VALGRIND.
```
?
💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2346256796)
> In general. if we have 0-value dust, I find it unlikely to hope that miners will clean it up for no reason?

Anyone can pay fees to clean up the dust. Doesn't have to be miners specifically.
👍 willcl-ark approved a pull request: "test: fix exclude parsing for functional runner"
(https://github.com/bitcoin/bitcoin/pull/30872#pullrequestreview-2300310873)
ACK 72b46f28bf8d37a166961b5aa2a22302b932b756

This fixes the described issue.

It would seem reasonable to introduce something to stop this reoccurring if possible. I added a commit [here](https://github.com/willcl-ark/bitcoin/commit/2be37ae18fa3daef78904dd751fe7da122c5bbcf) which prints out excluded tests at the end of the run in the results, but we could print excluded tests at the beginning as we remove them from the test_list too:

```log
TEST
...
💬 andrewtoth commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756868684)
This case will still have to be handled if this PR is not merged first:
https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L52-L55

> I would rather we simplify to boolean, and if we need more flags, change it to an uint8_t again

What I meant was adding a new field for DIRTY and a new field for FRESH. Since bools must be at least 1 byte it could make the pair larger depending on alignment.
⚠️ fanquake opened an issue: "ci: failure in feature_assumeutxo.py"
(https://github.com/bitcoin/bitcoin/issues/30878)
Seems like this is intermittently failing, since #30807 was merged? i.e https://cirrus-ci.com/task/4893652609138688?logs=ci#L3674. Seeing the same failure in PRs with unrelated changes, i.e https://cirrus-ci.com/task/5740917921939456.
💬 fanquake commented on issue "ci: failure in feature_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/issues/30878#issuecomment-2346300747)
Another failure in the same test: https://github.com/bitcoin/bitcoin/actions/runs/10829103648/job/30045925318?pr=30869#step:6:5514:
```bash
Traceback (most recent call last):
File "/home/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/runner/work/bitcoin/bitcoin/build/test/functional/feature_assumeutxo.py", line 738, in run_test
self.test_sync_from_assumeutxo_node(snapshot=dump_output)
File "/home/
...
💬 hebasto commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756879893)
Thank you! Accepted.
💬 hebasto commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2346306703)
My Guix build:
```
aarch64
d4baa1d2e9ee7abeb60d115dd52c6bc7d02e8116fe4b94c0f040371321e37b80 guix-build-253a691441dd/output/aarch64-linux-gnu/SHA256SUMS.part
7c38410b9468799b74371ba195f34121a4789e917dcf611f2ccc4b81f8a63eee guix-build-253a691441dd/output/aarch64-linux-gnu/bitcoin-253a691441dd-aarch64-linux-gnu-debug.tar.gz
4a6ec8df80822e5938b1a55f2e08a945b12a9d4f8b2f1bad05ea8be90a3b6173 guix-build-253a691441dd/output/aarch64-linux-gnu/bitcoin-253a691441dd-aarch64-linux-gnu.tar.gz
48de6fea
...
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2346309176)
re-ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678
No changes except variable naming and small doc update.
💬 fanquake commented on issue "ci: failure in feature_assumeutxo.py":
(https://github.com/bitcoin/bitcoin/issues/30878#issuecomment-2346309699)
Also seen here https://github.com/bitcoin/bitcoin/actions/runs/10828541638/job/30044139197#step:7:2918:
```bash
2024-09-12T13:21:45.762000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-x86_64-apple-darwin/test/functional/feature_assumeutxo.py", line 738, in run_test

...
💬 fanquake commented on pull request "Fix peers abruptly disconnecting from AssumeUTXO nodes during IBD":
(https://github.com/bitcoin/bitcoin/pull/30807#issuecomment-2346312892)
Any chance this is now causing #30878? If so, would be good to fix before this is backported.
👍 hebasto approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2300357639)
ACK 253a691441ddb04eac2b318112064e69f4b837b7.
📝 hebasto converted_to_draft a pull request: "build: Improve `ccache` performance for different build directories"
(https://github.com/bitcoin/bitcoin/pull/30861)
This PR makes it possible to reuse the `ccache` cache populated during a build in another build directory:
```
$ cmake -B build_1 -DWITH_CCACHE=ON
$ cmake --build build_1 -t bitcoind
$ cmake -B build_2 -DWITH_CCACHE=ON
$ cmake --build build_2 -t bitcoind # 100% cache hit rate is expected.
```

The first commit has been picked from https://github.com/bitcoin/bitcoin/pull/30803.

Please refer to https://github.com/bitcoin/bitcoin/pull/20353 for historical context.

Addresses this [com
...
👍 hodlinator approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300417228)
ACK fa5bc450d5d4c1d2daf7b205f1678402c3c21678

`git range-diff master fae8c25 fa5bc45`

Only minor comment adjustments and clearer variable names since last reviewed commits.

Since GitHub is [troublesome](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561525), would prefer this change if you re-touch:
```diff
- PassFmt<129>("%129$s 999$s %2$s");
+ PassFmt<12>("%12$s 999$s %2$s");
```
Makes it more reasonable to add comparison to tinyformat in follow-up with 12 arg
...
👍 TheCharlatan approved a pull request: "code style: update .editorconfig file"
(https://github.com/bitcoin/bitcoin/pull/30877#pullrequestreview-2300418237)
ACK e118dde75ad3a8e08051b4189e1e75c172a98043
👍 maflcko approved a pull request: "code style: update .editorconfig file"
(https://github.com/bitcoin/bitcoin/pull/30877#pullrequestreview-2300423885)
I don't use it, so I don't care, but the changes look plausible

lgtm