💬 l0rinc commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670192777)
Thanks for checking @Chand-ra - your mentioned examples are not "contains" alternatives but "exists" alternatives for `chrono::duration` (which is orthogonal to this change and doesn't seem to exist in C++20 anyway).
> Looks like GitHub nuked all historic commits and historic CI runs here?
I did push a few versions while CI was running, maybe that was confusing - is there anything you need me to do here?
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670192777)
Thanks for checking @Chand-ra - your mentioned examples are not "contains" alternatives but "exists" alternatives for `chrono::duration` (which is orthogonal to this change and doesn't seem to exist in C++20 anyway).
> Looks like GitHub nuked all historic commits and historic CI runs here?
I did push a few versions while CI was running, maybe that was confusing - is there anything you need me to do here?
💬 rkrux commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2631043584)
Thanks for the suggestions, updated PR to accommodate both of them.
(https://github.com/bitcoin/bitcoin/pull/32737#discussion_r2631043584)
Thanks for the suggestions, updated PR to accommodate both of them.
💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3670243771)
Actually, parsing the conf file should clear the old/stale datadir value:
```
src/common/config.cpp- // If datadir is changed in .conf file:
src/common/config.cpp: ClearPathCache();
src/common/config.cpp- if (!CheckDataDirOption(*this)) {
src/common/config.cpp- error = strprintf("specified data directory \"%s\" does not exist.", GetArg("-datadir", ""));
src/common/config.cpp- return false;
src/common/config.cpp- }
```
Can you reproduce this on Linux?
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3670243771)
Actually, parsing the conf file should clear the old/stale datadir value:
```
src/common/config.cpp- // If datadir is changed in .conf file:
src/common/config.cpp: ClearPathCache();
src/common/config.cpp- if (!CheckDataDirOption(*this)) {
src/common/config.cpp- error = strprintf("specified data directory \"%s\" does not exist.", GetArg("-datadir", ""));
src/common/config.cpp- return false;
src/common/config.cpp- }
```
Can you reproduce this on Linux?
💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670247073)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s
+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670247073)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s
+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...
💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670255939)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s
+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3670255939)
Still fails for me running the CI (via Podman 5.7.1) on my Fedora box (this PR rebased on master):
```bash
ALL | ✓ Passed | 6980 s (accumulated)
Runtime: 1243 s
+ traffic_monitor_end functional
+ test_name=functional
++ get_interfaces
++ set -o pipefail
++ ifconfig
++ awk -F ':| ' '/^[^[:space:]]/ { if (!match($1, /^lo/)) { print $1 } }'
++ set +o pipefail
+ for ifname in $(get_interfaces)
++ tcpdump_file functional eth0
++ loca
...
💬 rkrux commented on pull request "test: Improve code coverage for pubkey checks":
(https://github.com/bitcoin/bitcoin/pull/34099#issuecomment-3670257467)
Nice, the coverage is available now here: https://corecheck.dev/bitcoin/bitcoin/pulls/34099.
I can see the PR description mentioned functions of the interpreter file has increased coverage! I don't know why the previous PR didn't have the coverage available.
(https://github.com/bitcoin/bitcoin/pull/34099#issuecomment-3670257467)
Nice, the coverage is available now here: https://corecheck.dev/bitcoin/bitcoin/pulls/34099.
I can see the PR description mentioned functions of the interpreter file has increased coverage! I don't know why the previous PR didn't have the coverage available.
⚠️ fanquake opened an issue: "build: build broken with older supported Boost"
(https://github.com/bitcoin/bitcoin/issues/34101)
Broken by #33192.
Debian Bookworm
GCC 12.2.0
Boost 1.74.0 (our minimum supported is currently 1.73.0)
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Debug
<snip>
/bitcoin/src/txrequest.cpp: In member function 'void TxRequestTracker::Impl::ReceivedInv(NodeId, const GenTxid&, bool, std::chrono::microseconds)':
/bitcoin/src/txrequest.cpp:583:35: error: 'boost::multi_index::multi_index_container<{anonymous}::Announcement, {anonymous}::Announcement_Indices>::index<{anonymous}::ByPeer>::type' {aka 'class
...
(https://github.com/bitcoin/bitcoin/issues/34101)
Broken by #33192.
Debian Bookworm
GCC 12.2.0
Boost 1.74.0 (our minimum supported is currently 1.73.0)
```bash
cmake -B build -DCMAKE_BUILD_TYPE=Debug
<snip>
/bitcoin/src/txrequest.cpp: In member function 'void TxRequestTracker::Impl::ReceivedInv(NodeId, const GenTxid&, bool, std::chrono::microseconds)':
/bitcoin/src/txrequest.cpp:583:35: error: 'boost::multi_index::multi_index_container<{anonymous}::Announcement, {anonymous}::Announcement_Indices>::index<{anonymous}::ByPeer>::type' {aka 'class
...
💬 fanquake commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3670275347)
This has broken building against our minimum supported versions of Boost: #34101.
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3670275347)
This has broken building against our minimum supported versions of Boost: #34101.
📝 fanquake converted_to_draft a pull request: "refactor: enable `readability-container-contains` clang-tidy rule"
(https://github.com/bitcoin/bitcoin/pull/34095)
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* `boost::multi_index_container` (w)tx checks in `CTxMemPool::exists()`;
* other bare `std::unordered_set` and `std::map` count calls.
With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent f
...
(https://github.com/bitcoin/bitcoin/pull/34095)
Replace the last few instances of `.count() != 0` and `.count() == 0` and `.count()` patterns with the more expressive C++20 `.contains()` method:
* `std::set<std::string>` in `getblocktemplate` RPC;
* `std::map<std::string, ...>` in `transaction_tests`;
* `boost::multi_index_container` (w)tx checks in `CTxMemPool::exists()`;
* other bare `std::unordered_set` and `std::map` count calls.
With no remaining violations, enable the `readability-container-contains` clang-tidy check to prevent f
...
💬 fanquake commented on pull request "refactor: enable `readability-container-contains` clang-tidy rule":
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670288853)
Moved to draft for now, given these refactors have broken our minimum version requirements (#34101). That should be resolved before doing anything else here.
(https://github.com/bitcoin/bitcoin/pull/34095#issuecomment-3670288853)
Moved to draft for now, given these refactors have broken our minimum version requirements (#34101). That should be resolved before doing anything else here.
💬 hebasto commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670300267)
Is it a good time to consider bumping of the Boost minimum supported version?
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670300267)
Is it a good time to consider bumping of the Boost minimum supported version?
💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2631122111)
done.
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2631122111)
done.
💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2631122670)
Dropped it.
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2631122670)
Dropped it.
💬 0xB10C commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3670317642)
Addressed comments by mzumsande
(https://github.com/bitcoin/bitcoin/pull/34039#issuecomment-3670317642)
Addressed comments by mzumsande
💬 fanquake commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670335345)
Looks like it might have been added in Boost 1.78.0: https://github.com/boostorg/multi_index/issues/35.
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670335345)
Looks like it might have been added in Boost 1.78.0: https://github.com/boostorg/multi_index/issues/35.
💬 maflcko commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670376266)
> Is it a good time to consider bumping of the Boost minimum supported version?
I think there are users out there on Ubuntu:22.04 and Debian Bookworm, so breaking them seems a bit too early?
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670376266)
> Is it a good time to consider bumping of the Boost minimum supported version?
I think there are users out there on Ubuntu:22.04 and Debian Bookworm, so breaking them seems a bit too early?
💬 l0rinc commented on pull request "scripted-diff: [doc] Unify stale copyright headers":
(https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670396535)
This is tangentially related to https://github.com/bitcoin/bitcoin/issues/29445 - I don't really mind either way, I just want to stop the stupid headers distracting code review.
I have reviewed the change by:
* validating the regex 👍
* scrolling mindlessly through https://github.com/bitcoin/bitcoin/pull/34084/files?diff=unified 👍
* Manually checking https://patch-diff.githubusercontent.com/raw/bitcoin/bitcoin/pull/34084.patch and asking LLMs to proofread it as well 👍
This is officia
...
(https://github.com/bitcoin/bitcoin/pull/34084#issuecomment-3670396535)
This is tangentially related to https://github.com/bitcoin/bitcoin/issues/29445 - I don't really mind either way, I just want to stop the stupid headers distracting code review.
I have reviewed the change by:
* validating the regex 👍
* scrolling mindlessly through https://github.com/bitcoin/bitcoin/pull/34084/files?diff=unified 👍
* Manually checking https://patch-diff.githubusercontent.com/raw/bitcoin/bitcoin/pull/34084.patch and asking LLMs to proofread it as well 👍
This is officia
...
📝 fanquake opened a pull request: "depends: capnp 1.3.0"
(https://github.com/bitcoin/bitcoin/pull/34102)
Update capnp in depends to `1.3.0`.
Changes: https://github.com/capnproto/capnproto/compare/release-1.2.0...release-1.3.0.
(https://github.com/bitcoin/bitcoin/pull/34102)
Update capnp in depends to `1.3.0`.
Changes: https://github.com/capnproto/capnproto/compare/release-1.2.0...release-1.3.0.
💬 l0rinc commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670429071)
We could revert these instances, but in that case we have to make sure the CI catches these next time.
Let me know if there's anything you'd like me to do here.
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670429071)
We could revert these instances, but in that case we have to make sure the CI catches these next time.
Let me know if there's anything you'd like me to do here.
💬 marcofleon commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670432470)
> Looks like it might have been added in Boost 1.78.0
Just confirmed that 1.78.0 does build without errors.
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670432470)
> Looks like it might have been added in Boost 1.78.0
Just confirmed that 1.78.0 does build without errors.
💬 maflcko commented on issue "build: build broken with older supported Boost":
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670454317)
> We could revert these instances, but in that case we have to make sure the CI catches these next time.
I don't think it will be possible that CI catches "everything". In theory, the `ci/test/00_setup_env_native_previous_releases.sh` could be modified to run without depends to catch this, but then we wouldn't be checking depends. So a new ci config would have to be added. But that again will only check either gcc, or clang. So a new ci config will have to be added for both....
(https://github.com/bitcoin/bitcoin/issues/34101#issuecomment-3670454317)
> We could revert these instances, but in that case we have to make sure the CI catches these next time.
I don't think it will be possible that CI catches "everything". In theory, the `ci/test/00_setup_env_native_previous_releases.sh` could be modified to run without depends to catch this, but then we wouldn't be checking depends. So a new ci config would have to be added. But that again will only check either gcc, or clang. So a new ci config will have to be added for both....