💬 anointedboss1717 commented on pull request "doc: JSON-RPC request Content-Type is application/json":
(https://github.com/bitcoin/bitcoin/pull/30215#issuecomment-2181576338)
29947
(https://github.com/bitcoin/bitcoin/pull/30215#issuecomment-2181576338)
29947
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181583153)
> This job has a ccache size limit of 100mb, the MacOS job has a ccache limit of 400m. @hebasto is there something specific to MacOS for why the ccache limit is higher there?
When I adjusted ccache size limits, I picked a size that allowed to populate the cleared cache with no cleanups in the `ccache --show-stats` report. I'm not aware of any peculiarities that make macOS caches bigger than others.
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181583153)
> This job has a ccache size limit of 100mb, the MacOS job has a ccache limit of 400m. @hebasto is there something specific to MacOS for why the ccache limit is higher there?
When I adjusted ccache size limits, I picked a size that allowed to populate the cleared cache with no cleanups in the `ccache --show-stats` report. I'm not aware of any peculiarities that make macOS caches bigger than others.
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181589998)
> > Maybe wait for the CI to pass, then address the nits to test the created cache?
>
> Oh, I guess only master creates the cache?
>
> ```
> if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> ```
>
> So I guess this can be merged after addressing the nits.
Maybe comment that line out in a temporary "NO MERGE" commit?
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181589998)
> > Maybe wait for the CI to pass, then address the nits to test the created cache?
>
> Oh, I guess only master creates the cache?
>
> ```
> if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> ```
>
> So I guess this can be merged after addressing the nits.
Maybe comment that line out in a temporary "NO MERGE" commit?
📝 paplorinc opened a pull request: "WIP Optimize SipHash"
(https://github.com/bitcoin/bitcoin/pull/30317)
WIP, once all changes are done, I'll split this into small changes, I want to see first if CI passes for every scenario.
----------
Profiling `./bitcoind -reindex-chainstate -checkblocks=10000` revealed that hashing is a bottleneck:
<img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/8fa91d78-a5f0-4f4e-a281-f3ff1548b536">
The changes make it ~10% faster - while making the code more maintainable as well.
> make -j10 && src/test/test_bitcoin --run_test=hash_tests &
...
(https://github.com/bitcoin/bitcoin/pull/30317)
WIP, once all changes are done, I'll split this into small changes, I want to see first if CI passes for every scenario.
----------
Profiling `./bitcoind -reindex-chainstate -checkblocks=10000` revealed that hashing is a bottleneck:
<img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/8fa91d78-a5f0-4f4e-a281-f3ff1548b536">
The changes make it ~10% faster - while making the code more maintainable as well.
> make -j10 && src/test/test_bitcoin --run_test=hash_tests &
...
👍 tdb3 approved a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2131483027)
ACK 3f5027626593c7af546d3b53fc658c87bb815348
Helpful addition to help cover edge cases.
Anything that should be added to unit tests?
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2131483027)
ACK 3f5027626593c7af546d3b53fc658c87bb815348
Helpful addition to help cover edge cases.
Anything that should be added to unit tests?
💬 tdb3 commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1648208431)
Tested that this would fail if the `return util::Error...` line was commented out in source/wallet/spend.cpp. It did (as expected).
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1648208431)
Tested that this would fail if the `return util::Error...` line was commented out in source/wallet/spend.cpp. It did (as expected).
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2181689921)
Added a big description about the fuzzer serialization format for DepGraph objects.
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2181689921)
Added a big description about the fuzzer serialization format for DepGraph objects.
💬 ariard commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181708713)
> Possibly the best estimate we can get for that is to look at full-RBF
replacements that do not get mined. It's not too uncommon to see, for example,
OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
third highest fee-rate they were propagated at even though multiple minutes
have passed since the higher fee-rate txs were broadcast. I believe that is
evidence of poor propagation to miners on occasion.
This is good information to have. I’ll recall the original
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181708713)
> Possibly the best estimate we can get for that is to look at full-RBF
replacements that do not get mined. It's not too uncommon to see, for example,
OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
third highest fee-rate they were propagated at even though multiple minutes
have passed since the higher fee-rate txs were broadcast. I believe that is
evidence of poor propagation to miners on occasion.
This is good information to have. I’ll recall the original
...
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181796855)
> > Possibly the best estimate we can get for that is to look at full-RBF
> > replacements that do not get mined. It's not too uncommon to see, for example,
> > OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
> > third highest fee-rate they were propagated at even though multiple minutes
> > have passed since the higher fee-rate txs were broadcast. I believe that is
> > evidence of poor propagation to miners on occasion.
>
> This is good information to have.
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181796855)
> > Possibly the best estimate we can get for that is to look at full-RBF
> > replacements that do not get mined. It's not too uncommon to see, for example,
> > OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
> > third highest fee-rate they were propagated at even though multiple minutes
> > have passed since the higher fee-rate txs were broadcast. I believe that is
> > evidence of poor propagation to miners on occasion.
>
> This is good information to have.
...
🤔 tdb3 reviewed a pull request: "doc: clarify Cirrus self-hosted workers setup"
(https://github.com/bitcoin/bitcoin/pull/30314#pullrequestreview-2131695955)
Approach ACK.
This adds more clarity to CI config, much appreciated.
Left a comment and thought.
(https://github.com/bitcoin/bitcoin/pull/30314#pullrequestreview-2131695955)
Approach ACK.
This adds more clarity to CI config, much appreciated.
Left a comment and thought.
💬 tdb3 commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648353259)
These specs will become outdated over time (e.g. Ubuntu 23.04 which is already obsolete). Maybe instead it would be better to point the reader to `doc/dependencies.md`? `dependencies.md` doesn't currently specify distro versions, but aren't dependency versions really what matters, and distro version is a downstream consequence of that?
Perhaps this is a little less easy for the reader but helps prevent this file from becoming stale. Something like:
```diff
- # Generally, a persistent wor
...
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648353259)
These specs will become outdated over time (e.g. Ubuntu 23.04 which is already obsolete). Maybe instead it would be better to point the reader to `doc/dependencies.md`? `dependencies.md` doesn't currently specify distro versions, but aren't dependency versions really what matters, and distro version is a downstream consequence of that?
Perhaps this is a little less easy for the reader but helps prevent this file from becoming stale. Something like:
```diff
- # Generally, a persistent wor
...
💬 tdb3 commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648355507)
This one I'm a little torn about. Ideally, these machine specs would change less frequently than Linux distro versions (a side effect of keeping node requirements modest/reasonable), so maybe it makes sense to leave these specifics in this file.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648355507)
This one I'm a little torn about. Ideally, these machine specs would change less frequently than Linux distro versions (a side effect of keeping node requirements modest/reasonable), so maybe it makes sense to leave these specifics in this file.
💬 vasild commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181951994)
GitHub is convenient and free. IMO it is a vendor lock-in trap from which we will eventually want to get out. Adding more dependencies to it would make that harder.
"GitHub KYC is here boys!"
https://x.com/nitesh_btc/status/1802735626032210330
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181951994)
GitHub is convenient and free. IMO it is a vendor lock-in trap from which we will eventually want to get out. Adding more dependencies to it would make that harder.
"GitHub KYC is here boys!"
https://x.com/nitesh_btc/status/1802735626032210330
💬 vasild commented on issue "Make Transport independent of CNetMessage and CSerializedNetMsg":
(https://github.com/bitcoin/bitcoin/issues/30209#issuecomment-2181955144)
Concept ACK
(https://github.com/bitcoin/bitcoin/issues/30209#issuecomment-2181955144)
Concept ACK
⚠️ vasild opened an issue: "Fuzzing related configuration/build options can be improved"
(https://github.com/bitcoin/bitcoin/issues/30318)
### Please describe the feature you'd like to see added.
The fuzzing related configuration options are somewhat confusing and redundant and can be simplified.
### Is your feature related to a problem, if so please describe it.
Thanks for the [discussion](https://www.erisian.com.au/bitcoin-core-dev/log-2024-06-20.html#l-248) yesterday. Correct me if I am wrong below. There are the following options (to `./configure` and in the coming CMake replacement):
* `--enable-fuzz`: only disables comp
...
(https://github.com/bitcoin/bitcoin/issues/30318)
### Please describe the feature you'd like to see added.
The fuzzing related configuration options are somewhat confusing and redundant and can be simplified.
### Is your feature related to a problem, if so please describe it.
Thanks for the [discussion](https://www.erisian.com.au/bitcoin-core-dev/log-2024-06-20.html#l-248) yesterday. Correct me if I am wrong below. There are the following options (to `./configure` and in the coming CMake replacement):
* `--enable-fuzz`: only disables comp
...
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648454776)
This is just a moved sentence and still correct. 23.04+ is required, and should work, if someone manages to install it.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648454776)
This is just a moved sentence and still correct. 23.04+ is required, and should work, if someone manages to install it.
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648455689)
Also, your suggestion is incorrect. `doc/dependencies.md` has nothing to do with a CI system runner. In fact, you can probably use any distro, as long as you can install podman4.1+ or docker on it.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648455689)
Also, your suggestion is incorrect. `doc/dependencies.md` has nothing to do with a CI system runner. In fact, you can probably use any distro, as long as you can install podman4.1+ or docker on it.
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648456362)
Not sure what you mean. This is just a moved section to explain the three possible and required labels.
(https://github.com/bitcoin/bitcoin/pull/30314#discussion_r1648456362)
Not sure what you mean. This is just a moved section to explain the three possible and required labels.
💬 maflcko commented on pull request "doc: clarify Cirrus self-hosted workers setup":
(https://github.com/bitcoin/bitcoin/pull/30314#issuecomment-2182061790)
ACK 66b9740011222002ae9026e32e03a117628daae1
(https://github.com/bitcoin/bitcoin/pull/30314#issuecomment-2182061790)
ACK 66b9740011222002ae9026e32e03a117628daae1
💬 maflcko commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2182063190)
https://cirrus-ci.com/task/6263983722725376?logs=ci#L3388
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2182063190)
https://cirrus-ci.com/task/6263983722725376?logs=ci#L3388