💬 fanquake commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756779260)
When `-DBUILD_FOR_FUZZING=ON` is passed.
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756779260)
When `-DBUILD_FOR_FUZZING=ON` is passed.
💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346184415)
review ACK 72b46f28bf8d37a166961b5aa2a22302b932b756
(https://github.com/bitcoin/bitcoin/pull/30872#issuecomment-2346184415)
review ACK 72b46f28bf8d37a166961b5aa2a22302b932b756
👍 stickies-v approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300018207)
ACK fa37d9b0fd0b8f1b23163887618b2fa1a817d596
I like the new test structure and added documentation, thanks! Left some non-blocking nits, only if you need to force-push.
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2300018207)
ACK fa37d9b0fd0b8f1b23163887618b2fa1a817d596
I like the new test structure and added documentation, thanks! Left some non-blocking nits, only if you need to force-push.
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756687137)
nit: in this new new structure, `err_mix` makes more sense than `check_mix`
<details>
<summary>`s/check_/err_/g`</summary>
```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
index 4c61632fce..f398404d75 100644
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_string_tests.cpp
@@ -62,25 +62,25 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
PassFmt<2>("%2$*3$d");
PassFmt<1>("%.*f");
- auto check_mix{"Format specifiers
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756687137)
nit: in this new new structure, `err_mix` makes more sense than `check_mix`
<details>
<summary>`s/check_/err_/g`</summary>
```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_string_tests.cpp
index 4c61632fce..f398404d75 100644
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_string_tests.cpp
@@ -62,25 +62,25 @@ BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
PassFmt<2>("%2$*3$d");
PassFmt<1>("%.*f");
- auto check_mix{"Format specifiers
...
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756671121)
nit: `%8$` is not a valid specifier
```suggestion
// Positional specifier, like %8$s
```
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756671121)
nit: `%8$` is not a valid specifier
```suggestion
// Positional specifier, like %8$s
```
💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756690672)
+1 for focusing on run-time errors, formatting errors aren't worth spending much time on imo.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756690672)
+1 for focusing on run-time errors, formatting errors aren't worth spending much time on imo.
💬 fanquake commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2346192841)
Guix Build
```bash
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
48de6fea5e2d35be4
...
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2346192841)
Guix Build
```bash
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
48de6fea5e2d35be4
...
💬 hebasto commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756794139)
Could you please suggest your alternative?
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1756794139)
Could you please suggest your alternative?
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2346210098)
re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa
CI timeout issues seem unrelated.
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2346210098)
re-ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa
CI timeout issues seem unrelated.
📝 theStack opened a pull request: "code style: update .editorconfig file"
(https://github.com/bitcoin/bitcoin/pull/30877)
Updates the .editorconfig file, first introduced in 2021 (see PR #21123, commit 7a135d57) w.r.t. following changes:
- consider Rust .rs files (relevant since #28076, commit bbbbdb0c)
- reflect build system change to CMake (#30454, #30664)
- add setting for bare Makefile still used for depends builds
Can be tested e.g. by using the editorconfig-vim plugin (https://github.com/editorconfig/editorconfig-vim). The PR is made under the assumption that the file is still considered useful, especia
...
(https://github.com/bitcoin/bitcoin/pull/30877)
Updates the .editorconfig file, first introduced in 2021 (see PR #21123, commit 7a135d57) w.r.t. following changes:
- consider Rust .rs files (relevant since #28076, commit bbbbdb0c)
- reflect build system change to CMake (#30454, #30664)
- add setting for bare Makefile still used for depends builds
Can be tested e.g. by using the editorconfig-vim plugin (https://github.com/editorconfig/editorconfig-vim). The PR is made under the assumption that the file is still considered useful, especia
...
💬 hodlinator commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2346225294)
I put some time into implementing 3 variations on different personal branches:
### [unchanged_key_CDiskTxPos](https://github.com/bitcoin/bitcoin/compare/20b259332ff1d2e6ca6b291ffeda00b51497bbed...hodlinator:bitcoin:pr/24539_unchanged_key_CDiskTxPos) / baf57fb3a45c97d6cfb531b9252c97d4f27ad7ae
`uint256` value -> `CDiskTxPos`
Still have key of `COutPoint`: 32 + 4 = 36 bytes
old value - `uint256`: 32 bytes
new value - `CDiskTxPos`: 4 + 4 + 4 = 12 bytes
36+32 -> 36+12 => 29% theoretical
...
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2346225294)
I put some time into implementing 3 variations on different personal branches:
### [unchanged_key_CDiskTxPos](https://github.com/bitcoin/bitcoin/compare/20b259332ff1d2e6ca6b291ffeda00b51497bbed...hodlinator:bitcoin:pr/24539_unchanged_key_CDiskTxPos) / baf57fb3a45c97d6cfb531b9252c97d4f27ad7ae
`uint256` value -> `CDiskTxPos`
Still have key of `COutPoint`: 32 + 4 = 36 bytes
old value - `uint256`: 32 bytes
new value - `CDiskTxPos`: 4 + 4 + 4 = 12 bytes
36+32 -> 36+12 => 29% theoretical
...
🤔 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.
(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
...
(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?
(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.
```
?
(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.
(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
...
(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.
(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.
(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/
...
(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/
...