👍 tdb3 approved a pull request: "refactor: move util/pcp and util/netif to common/"
(https://github.com/bitcoin/bitcoin/pull/31011#pullrequestreview-2341666390)
ACK fd38711217cafbd62e8abd22d2b43f85fede8cde
cherry-pick'ed fd38711217cafbd62e8abd22d2b43f85fede8cde into master (intentionally without d51edecddcb7fa52349c8aa5ef88b01f72be44c7), ran `check-deps.sh`, and verified (with `echo $?`) that the script exited with 1 (vs 0 on master without the cherry-pick'ed commit).
(https://github.com/bitcoin/bitcoin/pull/31011#pullrequestreview-2341666390)
ACK fd38711217cafbd62e8abd22d2b43f85fede8cde
cherry-pick'ed fd38711217cafbd62e8abd22d2b43f85fede8cde into master (intentionally without d51edecddcb7fa52349c8aa5ef88b01f72be44c7), ran `check-deps.sh`, and verified (with `echo $?`) that the script exited with 1 (vs 0 on master without the cherry-pick'ed commit).
💬 tdb3 commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#discussion_r1783673649)
femto nit. Ignore if not already updating the file for something else.
```diff
- #include <common/pcp.h>
- #include <common/netif.h>
+ #include <common/netif.h>
+ #include <common/pcp.h>
```
(https://github.com/bitcoin/bitcoin/pull/31011#discussion_r1783673649)
femto nit. Ignore if not already updating the file for something else.
```diff
- #include <common/pcp.h>
- #include <common/netif.h>
+ #include <common/netif.h>
+ #include <common/pcp.h>
```
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783599080)
Added the `Assume`. I agree it makes more sense.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783599080)
Added the `Assume`. I agree it makes more sense.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783611446)
Ah, sorry for missing that. I'm not really convinced this is more efficient overall?
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783611446)
Ah, sorry for missing that. I'm not really convinced this is more efficient overall?
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783578259)
edited somewhat. I like the annotations there but only did first row.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783578259)
edited somewhat. I like the annotations there but only did first row.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783602138)
Thanks, done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783602138)
Thanks, done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783604279)
More efficient to shuffle ints than objects. I've changed to use the ctor with the size.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783604279)
More efficient to shuffle ints than objects. I've changed to use the ctor with the size.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783572311)
edited to no longer be for a chain
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783572311)
edited to no longer be for a chain
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783604666)
done
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783604666)
done
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783602611)
Unsure, it's just a habit I guess.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783602611)
Unsure, it's just a habit I guess.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783609907)
This is part of a block of code that was moved from `PeerManagerImpl`. The members still have the same name, so I don't think this is incorrect.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783609907)
This is part of a block of code that was moved from `PeerManagerImpl`. The members still have the same name, so I don't think this is incorrect.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783569179)
added
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1783569179)
added
💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2387502458)
Pushed to b6368fc285bf00b3033061dcd4e29298b227c6df (rebased on top of fc642c33ef28829eda0119a0fe39fd9bc4b84051)
<details>
<summary>Saw the following CI error for macOS 14 with e6853592361341c27103ed74b25470ac1e098d6d</summary>
This seems to be related to PR #30684
```
node0 2024-10-02T00:15:45.783638Z [init] [src/noui.cpp:57] [noui_InitMessage] init message: Verifying wallet(s)…
Error: node0 2024-10-02T00:15:45.783653Z [init] [src/noui.cpp:31] [noui_ThreadSafeMessageBox] [error] In
...
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2387502458)
Pushed to b6368fc285bf00b3033061dcd4e29298b227c6df (rebased on top of fc642c33ef28829eda0119a0fe39fd9bc4b84051)
<details>
<summary>Saw the following CI error for macOS 14 with e6853592361341c27103ed74b25470ac1e098d6d</summary>
This seems to be related to PR #30684
```
node0 2024-10-02T00:15:45.783638Z [init] [src/noui.cpp:57] [noui_InitMessage] init message: Verifying wallet(s)…
Error: node0 2024-10-02T00:15:45.783653Z [init] [src/noui.cpp:31] [noui_ThreadSafeMessageBox] [error] In
...
💬 luisschwab commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2387536123)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2387536123)
Concept ACK
💬 maflcko commented on pull request "test: add missing sync to feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/31016#issuecomment-2387683977)
lgtm ACK a1576edab356053c4c736691e4950b58e9a14f76
(https://github.com/bitcoin/bitcoin/pull/31016#issuecomment-2387683977)
lgtm ACK a1576edab356053c4c736691e4950b58e9a14f76
💬 maflcko commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2387699942)
> I added a commit to bypass some asserts in `ConnmanTestMsg` that would fail since we're bypassing network magic and checksum validation. Also, I think we will have to change `p2p_transport_bidirectional` target since there are some sanity checks that would fail with the bypasses, especially the assertions about `ReceivedBytes`/`GetReceivedMessage`. Any thoughts?
Can you explain why the asserts and sanity checks would be failing? They are unrelated to the checksum, so if they are failing, it
...
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2387699942)
> I added a commit to bypass some asserts in `ConnmanTestMsg` that would fail since we're bypassing network magic and checksum validation. Also, I think we will have to change `p2p_transport_bidirectional` target since there are some sanity checks that would fail with the bypasses, especially the assertions about `ReceivedBytes`/`GetReceivedMessage`. Any thoughts?
Can you explain why the asserts and sanity checks would be failing? They are unrelated to the checksum, so if they are failing, it
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2387782120)
@ryanofsky that worked!
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2387782120)
@ryanofsky that worked!
💬 maflcko commented on issue "ci: failure in win64 unit tests":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2387848251)
I was able to reproduce locally (in a CI container), but given that there is no useful output, it doesn't help:
```
# while ( LC_ALL=C.UTF-8 DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/ ctest --test-dir /ci_container_base/ci/scratch/build-x86_64-w64-mingw32 -j $( nproc ) ) ; do ( echo 1 >> /tmp/runs ) ; done
...
# cat /tmp/runs |wc -l
48
```
The log:
```
...
1/136 Testing: util_test_runner
1/136 Test: util_test_runner
Command: "/usr/bin/cmake
...
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2387848251)
I was able to reproduce locally (in a CI container), but given that there is no useful output, it doesn't help:
```
# while ( LC_ALL=C.UTF-8 DIR_UNIT_TEST_DATA=/ci_container_base/ci/scratch/qa-assets/unit_test_data/ ctest --test-dir /ci_container_base/ci/scratch/build-x86_64-w64-mingw32 -j $( nproc ) ) ; do ( echo 1 >> /tmp/runs ) ; done
...
# cat /tmp/runs |wc -l
48
```
The log:
```
...
1/136 Testing: util_test_runner
1/136 Test: util_test_runner
Command: "/usr/bin/cmake
...
💬 laanwj commented on pull request "refactor: move util/pcp and util/netif to common/":
(https://github.com/bitcoin/bitcoin/pull/31011#discussion_r1784030205)
even better, can remove the include of `netif.h` here, it's not actually used in `pcp.cpp`
(https://github.com/bitcoin/bitcoin/pull/31011#discussion_r1784030205)
even better, can remove the include of `netif.h` here, it's not actually used in `pcp.cpp`
💬 maflcko commented on issue "ci: failure in win64 unit tests":
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2387925869)
Probably the fastest way to reproduce for now inside the CI container:
```
# while ( LC_ALL=C.UTF-8 BITCOINUTIL=/ci_container_base/ci/scratch/build-x86_64-w64-mingw32/src/bitcoin-util.exe BITCOINTX=/ci_container_base/ci/scratch/build-x86_64-w64-mingw32/src/bitcoin-tx.exe /usr/bin/python3 /ci_container_base/ci/scratch/build-x86_64-w64-mingw32/test/util/test_runner.py ) ; do ( echo '...' && echo 1 >> /tmp/runs ) ; done
...
...
...
...
...
...
...
...
...
...
...
...
...
...
...
...
(https://github.com/bitcoin/bitcoin/issues/30792#issuecomment-2387925869)
Probably the fastest way to reproduce for now inside the CI container:
```
# while ( LC_ALL=C.UTF-8 BITCOINUTIL=/ci_container_base/ci/scratch/build-x86_64-w64-mingw32/src/bitcoin-util.exe BITCOINTX=/ci_container_base/ci/scratch/build-x86_64-w64-mingw32/src/bitcoin-tx.exe /usr/bin/python3 /ci_container_base/ci/scratch/build-x86_64-w64-mingw32/test/util/test_runner.py ) ; do ( echo '...' && echo 1 >> /tmp/runs ) ; done
...
...
...
...
...
...
...
...
...
...
...
...
...
...
...
...