💬 fanquake commented on pull request "fuzz: Rename fuzz_seed_corpus to fuzz_corpora":
(https://github.com/bitcoin/bitcoin/pull/30804#issuecomment-2328307469)
cc @murchandamus @dergoegge
(https://github.com/bitcoin/bitcoin/pull/30804#issuecomment-2328307469)
cc @murchandamus @dergoegge
💬 maflcko commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743353913)
I don't recall when wine was ever working. There was always a a number of intermittent or consistent errors, regardless of cmake, see https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632851441
Fixing that seems unrelated to the cmake transition.
I think simply removing the accidentally added line is fine and other follow-ups can be done in a follow-up, if they are needed.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1743353913)
I don't recall when wine was ever working. There was always a a number of intermittent or consistent errors, regardless of cmake, see https://github.com/bitcoin/bitcoin/pull/29881#discussion_r1632851441
Fixing that seems unrelated to the cmake transition.
I think simply removing the accidentally added line is fine and other follow-ups can be done in a follow-up, if they are needed.
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2328325006)
Rebased after #26619.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2328325006)
Rebased after #26619.
💬 remyers commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328327755)
Fwiw, I also ran across this problem (Ubuntu 20.04) while trying to run `clang-tidy-diff` on my code. A workaround is to just install and use clang-17.
```
cmake -B build -DCMAKE_C_COMPILER=clang-17 -DCMAKE_CXX_COMPILER=clang++-17 -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
cmake --build build -j $(nproc)
git diff HEAD~2 | ( cd ./src/ && clang-tidy-diff -p2 -path ../build -j $(nproc) )
```
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328327755)
Fwiw, I also ran across this problem (Ubuntu 20.04) while trying to run `clang-tidy-diff` on my code. A workaround is to just install and use clang-17.
```
cmake -B build -DCMAKE_C_COMPILER=clang-17 -DCMAKE_CXX_COMPILER=clang++-17 -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
cmake --build build -j $(nproc)
git diff HEAD~2 | ( cd ./src/ && clang-tidy-diff -p2 -path ../build -j $(nproc) )
```
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2328330492)
Rebased after trivial conflict with #29553.
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2328330492)
Rebased after trivial conflict with #29553.
👍 fanquake approved a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802#pullrequestreview-2279470892)
ACK fa78ed83be1f6831416a6f6632e2161f12d359e4
(https://github.com/bitcoin/bitcoin/pull/30802#pullrequestreview-2279470892)
ACK fa78ed83be1f6831416a6f6632e2161f12d359e4
🚀 fanquake merged a pull request: "doc: Clarify libbitcoin_consensus in design/libraries.md"
(https://github.com/bitcoin/bitcoin/pull/30802)
(https://github.com/bitcoin/bitcoin/pull/30802)
📝 maflcko opened a pull request: "lint: Check for release note snippets in the wrong folder"
(https://github.com/bitcoin/bitcoin/pull/30812)
It is a common mistake to place the snippets in the wrong folder, where they could be missed. For example https://github.com/bitcoin/bitcoin/pull/30719#pullrequestreview-2262535007 or commit 84900ac34f6888b7a851d0a6a5885192155f865c.
Fix all issues by adding a simple lint check.
Can be tested by reverting a prior commit that violated the rule and then running the new check:
```
git revert 35ef34eab7b36e3c53ed438d74a9b783cbcaec27
( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo
...
(https://github.com/bitcoin/bitcoin/pull/30812)
It is a common mistake to place the snippets in the wrong folder, where they could be missed. For example https://github.com/bitcoin/bitcoin/pull/30719#pullrequestreview-2262535007 or commit 84900ac34f6888b7a851d0a6a5885192155f865c.
Fix all issues by adding a simple lint check.
Can be tested by reverting a prior commit that violated the rule and then running the new check:
```
git revert 35ef34eab7b36e3c53ed438d74a9b783cbcaec27
( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo
...
💬 maflcko commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328378759)
> Fwiw, I also ran across this problem (Ubuntu 20.04)
I guess you also installed a different boost version, because the 1.71 boost version in Ubuntu Focal 20.04 is not supported starting with https://github.com/bitcoin/bitcoin/pull/29066 and shouldn't even pass the `cmake -B` command.
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328378759)
> Fwiw, I also ran across this problem (Ubuntu 20.04)
I guess you also installed a different boost version, because the 1.71 boost version in Ubuntu Focal 20.04 is not supported starting with https://github.com/bitcoin/bitcoin/pull/29066 and shouldn't even pass the `cmake -B` command.
💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743476276)
For reference, the len reporting (and error handling) was added after https://github.com/bitcoin/bitcoin/issues/9040#issuecomment-257161620.
Also, you added len reporting in a similar place recently in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499, so I fail to see why there it is "helpful" while here it would be not.
I am happy to review and ACK either version, but I think the length can be useful and may be better to not remove from the error string.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743476276)
For reference, the len reporting (and error handling) was added after https://github.com/bitcoin/bitcoin/issues/9040#issuecomment-257161620.
Also, you added len reporting in a similar place recently in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499, so I fail to see why there it is "helpful" while here it would be not.
I am happy to review and ACK either version, but I think the length can be useful and may be better to not remove from the error string.
💬 maflcko commented on pull request "refactor: Replace ParseHex with consteval ""_hex literals":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2328519108)
macOS follow-up idea: Someone could check on macOS 13, if compilation with XCode still works. If not, `build-osx.md` could be updated (Ref: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716956530)
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2328519108)
macOS follow-up idea: Someone could check on macOS 13, if compilation with XCode still works. If not, `build-osx.md` could be updated (Ref: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716956530)
💬 fanquake commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1743507523)
It's disappointing that so many distros don't support CMake in their packaging. We basically can't switch this (and any other packages where we might want to use the "native" CMake config) until *every* distro does, otherwise it'll just be broken for that distro.
Some do, i.e [Alpine](https://pkgs.alpinelinux.org/contents?branch=edge&name=zeromq%2ddev&arch=x86&repo=main), or [Arch](https://archlinux.org/packages/extra/x86_64/zeromq/), or [nix](https://github.com/NixOS/nixpkgs/blob/release-23.
...
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1743507523)
It's disappointing that so many distros don't support CMake in their packaging. We basically can't switch this (and any other packages where we might want to use the "native" CMake config) until *every* distro does, otherwise it'll just be broken for that distro.
Some do, i.e [Alpine](https://pkgs.alpinelinux.org/contents?branch=edge&name=zeromq%2ddev&arch=x86&repo=main), or [Arch](https://archlinux.org/packages/extra/x86_64/zeromq/), or [nix](https://github.com/NixOS/nixpkgs/blob/release-23.
...
💬 GBKS commented on pull request "Show transactions as not fully confirmed during background validation":
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2328544171)
Was just thinking that an alternative approach could be to leave the transactions list as is, but show a message in the send screen that explains the current status and trust assumptions. This is much more explicit than slightly tweaked icons. Just wanted to add it as an idea for an alternative approach, feel free to ignore for the purpose of this PR.
(https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2328544171)
Was just thinking that an alternative approach could be to leave the transactions list as is, but show a message in the send screen that explains the current status and trust assumptions. This is much more explicit than slightly tweaked icons. Just wanted to add it as an idea for an alternative approach, feel free to ignore for the purpose of this PR.
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743536308)
> Also, you added len reporting in a similar place recently in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499,
I don't quite think this is similar. My suggestion in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499 was to use less code to have more accurate and helpful error reporting, triggered only when that particular requirement is breached. In this PR, we're talking about adding more code to provide context that may not be relevant (and thus make
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743536308)
> Also, you added len reporting in a similar place recently in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499,
I don't quite think this is similar. My suggestion in https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1717324499 was to use less code to have more accurate and helpful error reporting, triggered only when that particular requirement is breached. In this PR, we're talking about adding more code to provide context that may not be relevant (and thus make
...
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2328592993)
Force-pushed to remove behaviour change in commit "rpc: use uint256::FromHex for ParseHashV", addressing @maflcko's [concerns](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1738970588) over behaviour change. I don't think this is necessarily an improvement, but I appreciate that's my subjective view and the (slight) behaviour change was orthogonal the goal of the PR anyway so it seems like the most pragmatic approach to make progress. Thanks for your review!
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2328592993)
Force-pushed to remove behaviour change in commit "rpc: use uint256::FromHex for ParseHashV", addressing @maflcko's [concerns](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1738970588) over behaviour change. I don't think this is necessarily an improvement, but I appreciate that's my subjective view and the (slight) behaviour change was orthogonal the goal of the PR anyway so it seems like the most pragmatic approach to make progress. Thanks for your review!
👍 marcofleon approved a pull request: "fuzz: Rename fuzz_seed_corpus to fuzz_corpora"
(https://github.com/bitcoin/bitcoin/pull/30804#pullrequestreview-2279780062)
ACK 8888beea8d477b1d4a2dfd2a0bb5f686de62f3ff
(https://github.com/bitcoin/bitcoin/pull/30804#pullrequestreview-2279780062)
ACK 8888beea8d477b1d4a2dfd2a0bb5f686de62f3ff
💬 remyers commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328622998)
> > Fwiw, I also ran across this problem (Ubuntu 20.04)
>
> I guess you also installed a different boost version, because the 1.71 boost version in Ubuntu Focal 20.04 is not supported starting with #29066 and shouldn't even pass the `cmake -B` command.
Yup, I have boost 1.74.0.3 installed.
```sh
dpkg -s libboost-dev | grep 'Version'
Version: 1.74.0.3ubuntu7
```
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2328622998)
> > Fwiw, I also ran across this problem (Ubuntu 20.04)
>
> I guess you also installed a different boost version, because the 1.71 boost version in Ubuntu Focal 20.04 is not supported starting with #29066 and shouldn't even pass the `cmake -B` command.
Yup, I have boost 1.74.0.3 installed.
```sh
dpkg -s libboost-dev | grep 'Version'
Version: 1.74.0.3ubuntu7
```
💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2328685708)
> Concept ACK
>
> I agree that this would be interesting. The the orphan pool is mostly a black box to me (and probably others too). To discuss improvements/changes, having a better visual and mental model would be helpful. Maybe even similar to `getrawaddrman` and https://addrman.observer.
Thanks. That web app is gorgeous! Any particular tx details you'd like to see added (e.g. orphan expiration, etc.)?
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2328685708)
> Concept ACK
>
> I agree that this would be interesting. The the orphan pool is mostly a black box to me (and probably others too). To discuss improvements/changes, having a better visual and mental model would be helpful. Maybe even similar to `getrawaddrman` and https://addrman.observer.
Thanks. That web app is gorgeous! Any particular tx details you'd like to see added (e.g. orphan expiration, etc.)?
👋 tdb3's pull request is ready for review: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793)
(https://github.com/bitcoin/bitcoin/pull/30793)
💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2328715995)
> Today's mempool does a suboptimal job of dealing with chain limits and RBFs. Assuming cluster mempool, we still have the issue of what to do with cluster limits that are exceeded even after removing RBF conflicts, if any. This has been called "sibling eviction" and in general seems doable in a DoS-resistant way but is left to future work. It's a problem I'm very interested in solving post-cluster mempool.
What I mean is, if you are doing an RBF replacement of one transaction with another tr
...
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2328715995)
> Today's mempool does a suboptimal job of dealing with chain limits and RBFs. Assuming cluster mempool, we still have the issue of what to do with cluster limits that are exceeded even after removing RBF conflicts, if any. This has been called "sibling eviction" and in general seems doable in a DoS-resistant way but is left to future work. It's a problem I'm very interested in solving post-cluster mempool.
What I mean is, if you are doing an RBF replacement of one transaction with another tr
...