🚀 fanquake merged a pull request: "ci: Add missed configuration options to "Win64 native" job"
(https://github.com/bitcoin/bitcoin/pull/30755)
(https://github.com/bitcoin/bitcoin/pull/30755)
💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1747106280)
> adding negative value to the values makes this test to fail. should we also prevent passing negative integers?
`-nowallet=-1` should be treated the same as `-nowallet=1` which is the same as `-nowallet` which just clears the lists of wallets to load and is not expected to be an error.
This test is testing what happens when `-nowallet=0` is passed. Previously before https://github.com/bitcoin/bitcoin/pull/22217 `-nowallet=0` was the treated the same as `-wallet=1`, and would cause a walle
...
(https://github.com/bitcoin/bitcoin/pull/30684#discussion_r1747106280)
> adding negative value to the values makes this test to fail. should we also prevent passing negative integers?
`-nowallet=-1` should be treated the same as `-nowallet=1` which is the same as `-nowallet` which just clears the lists of wallets to load and is not expected to be an error.
This test is testing what happens when `-nowallet=0` is passed. Previously before https://github.com/bitcoin/bitcoin/pull/22217 `-nowallet=0` was the treated the same as `-wallet=1`, and would cause a walle
...
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334033449)
Examples 16-19 seem to get slower?
My results on the last commit look similar to sipa's:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 17,187,614.00 | 58.18 | 4.0% | 0.19 | `LinearizeOptimallyExample00`
| 96,365.70 | 10,377.14 | 0.2% | 0.01 | `LinearizeOptimallyExample01`
| 87,832.50 | 11,385.31 | 3
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334033449)
Examples 16-19 seem to get slower?
My results on the last commit look similar to sipa's:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 17,187,614.00 | 58.18 | 4.0% | 0.19 | `LinearizeOptimallyExample00`
| 96,365.70 | 10,377.14 | 0.2% | 0.01 | `LinearizeOptimallyExample01`
| 87,832.50 | 11,385.31 | 3
...
💬 fanquake commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334037507)
> Do you still happen to have the compile failure or steps to reproduce?
Using this Clang:
```bash
clang++-20 --version
Debian clang version 20.0.0 (++20240903115437+0748f4227cd6-1~exp1~20240903115620.935)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/lib/llvm-20/bin
```
and master (bbf95c0cc57147827b9f4577c641b12dd4170e78). I see:
```bash
cmake -B build -DCMAKE_C_COMPILER=clang-20 -DCMAKE_CXX_COMPILER=clang++-20
cmake --build build --verbose
<snip>
[ 72%] Bu
...
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334037507)
> Do you still happen to have the compile failure or steps to reproduce?
Using this Clang:
```bash
clang++-20 --version
Debian clang version 20.0.0 (++20240903115437+0748f4227cd6-1~exp1~20240903115620.935)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/lib/llvm-20/bin
```
and master (bbf95c0cc57147827b9f4577c641b12dd4170e78). I see:
```bash
cmake -B build -DCMAKE_C_COMPILER=clang-20 -DCMAKE_CXX_COMPILER=clang++-20
cmake --build build --verbose
<snip>
[ 72%] Bu
...
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334042896)
Thanks for the graphs, @glozow. That confirms the impression we got: 16-19 seem to just be really hard, and the later optimizations just don't help with it (and increase per-iteration cost, for no meaningful iteration reduction).
It doesn't surprise me that this is possible (they are after all very far below the conjectured upper bound of $\mathcal{O}(\sqrt{2^N})$, but it does surprise me that such clusters are seen in historical real-world mempool data.
Perhaps it's worth investigating (in a
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334042896)
Thanks for the graphs, @glozow. That confirms the impression we got: 16-19 seem to just be really hard, and the later optimizations just don't help with it (and increase per-iteration cost, for no meaningful iteration reduction).
It doesn't surprise me that this is possible (they are after all very far below the conjectured upper bound of $\mathcal{O}(\sqrt{2^N})$, but it does surprise me that such clusters are seen in historical real-world mempool data.
Perhaps it's worth investigating (in a
...
💬 fanquake commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2334046532)
cc @stickies-v @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2334046532)
cc @stickies-v @ryanofsky
💬 maflcko commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334046805)
Ok, I may have only compiled `bitcoind`. If the error is only in the tests, I wonder if the tests can simply be rewritten. I'll take another look later.
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334046805)
Ok, I may have only compiled `bitcoind`. If the error is only in the tests, I wonder if the tests can simply be rewritten. I'll take another look later.
💬 ryanofsky commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747126073)
> An overload for `std::byte` could be one more line of code, but would avoid having to touch all the touched non-header lines in this pull request again.
Yeah sorry for any confusion. My suggestion in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741079145 here was to use `std::byte` if it was easy or `BasicByte` as a fallback if would be too much work to convert other code to use std::byte. Having two overloads is also fine.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747126073)
> An overload for `std::byte` could be one more line of code, but would avoid having to touch all the touched non-header lines in this pull request again.
Yeah sorry for any confusion. My suggestion in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1741079145 here was to use `std::byte` if it was easy or `BasicByte` as a fallback if would be too much work to convert other code to use std::byte. Having two overloads is also fine.
💬 maflcko commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`, too":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747142070)
> `BasicByte`
IIRC the reason that serialize code uses a template on the inner type of the span is that call sites have to explicitly specify that they want the "different" span-serialization.
I think the concern doesn't apply to CScript, so a template may be better to avoid, but I haven't tried this myself.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1747142070)
> `BasicByte`
IIRC the reason that serialize code uses a template on the inner type of the span is that call sites have to explicitly specify that they want the "different" span-serialization.
I think the concern doesn't apply to CScript, so a template may be better to avoid, but I haven't tried this myself.
💬 andrewtoth commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2334078040)
Reason I suspect #30326 is because `try_emplace` must be doing more work than just `find` and is only called after blocks stop being full because previous coins are now being looked up to be spent.
Relevant stackoverflow https://stackoverflow.com/questions/24209592/performance-of-emplace-is-worse-than-check-followed-by-emplace
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2334078040)
Reason I suspect #30326 is because `try_emplace` must be doing more work than just `find` and is only called after blocks stop being full because previous coins are now being looked up to be spent.
Relevant stackoverflow https://stackoverflow.com/questions/24209592/performance-of-emplace-is-worse-than-check-followed-by-emplace
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2334086913)
CI failures seem like spurious timeouts of random tests
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2334086913)
CI failures seem like spurious timeouts of random tests
💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334102610)
> @sdaftuar Is it possible that some of these clusters are "related" (as in are really modifications of one another with a few transactions added/removed)? That could explain the strong correlations between consecutive ones.
I do believe those last 4 examples are likely to be related, as all were found within minutes of each other on the same day.
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2334102610)
> @sdaftuar Is it possible that some of these clusters are "related" (as in are really modifications of one another with a few transactions added/removed)? That could explain the strong correlations between consecutive ones.
I do believe those last 4 examples are likely to be related, as all were found within minutes of each other on the same day.
💬 ryanofsky commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1747172543)
In commit "chain: disallow overwriting settings with null values" (4627174d80838e1c6848f9dff47a2ca4a9a35091)
Would be good to just delete this line entirely and make overwriteRwSetting just call updateRwSetting without making a special case for null. Setting a null setting should just delete a setting consistently and not have different behavior for different functions.
(Sorry I didn't notice this line before and mistakenly thought overwriteRwSetting would allow writing null values to the
...
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1747172543)
In commit "chain: disallow overwriting settings with null values" (4627174d80838e1c6848f9dff47a2ca4a9a35091)
Would be good to just delete this line entirely and make overwriteRwSetting just call updateRwSetting without making a special case for null. Setting a null setting should just delete a setting consistently and not have different behavior for different functions.
(Sorry I didn't notice this line before and mistakenly thought overwriteRwSetting would allow writing null values to the
...
📝 maflcko opened a pull request: " test: Work around boost compilation error "
(https://github.com/bitcoin/bitcoin/pull/30834)
(https://github.com/bitcoin/bitcoin/pull/30834)
💬 Sjors commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2334121626)
re-ACK e7cf8e8fc658e4559fcc64279c7bd854773ad6c2
This makes my dimmed zebra very happy.
(https://github.com/bitcoin/bitcoin/pull/30205#issuecomment-2334121626)
re-ACK e7cf8e8fc658e4559fcc64279c7bd854773ad6c2
This makes my dimmed zebra very happy.
💬 maflcko commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334122738)
I still haven't tested this, but the draft at https://github.com/bitcoin/bitcoin/pull/30834 may or may not work around the test compilation error.
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2334122738)
I still haven't tested this, but the draft at https://github.com/bitcoin/bitcoin/pull/30834 may or may not work around the test compilation error.
💬 fanquake commented on pull request "test: Work around boost compilation error":
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334129103)
It looks like this works using clang 20 (`Debian clang version 20.0.0 (++20240903115437+0748f4227cd6-1~exp1~20240903115620.935)`) & Boost 1.74.0.
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334129103)
It looks like this works using clang 20 (`Debian clang version 20.0.0 (++20240903115437+0748f4227cd6-1~exp1~20240903115620.935)`) & Boost 1.74.0.
👋 maflcko's pull request is ready for review: "test: Work around boost compilation error"
(https://github.com/bitcoin/bitcoin/pull/30834)
(https://github.com/bitcoin/bitcoin/pull/30834)
💬 maflcko commented on pull request "test: Work around boost compilation error":
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334133682)
Thanks for testing. It would be good to also check with Clang 18 (and any other known-to-fail combination).
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334133682)
Thanks for testing. It would be good to also check with Clang 18 (and any other known-to-fail combination).
💬 fanquake commented on pull request "test: Work around boost compilation error":
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334134054)
Note also, I somehow discovered only now that there is a separate Boost 1.81.0 package available for Bookworm, and the same package is in the backports for Bullseye. So we can also direct any users to those packages.
(https://github.com/bitcoin/bitcoin/pull/30834#issuecomment-2334134054)
Note also, I somehow discovered only now that there is a separate Boost 1.81.0 package available for Bookworm, and the same package is in the backports for Bullseye. So we can also direct any users to those packages.