👍 i-am-yuvi approved a pull request: "net: Use actual memory size in receive buffer accounting"
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2417660993)
ACK d22a234ed270286b483aec2db1e2f716b9756231
(https://github.com/bitcoin/bitcoin/pull/31164#pullrequestreview-2417660993)
ACK d22a234ed270286b483aec2db1e2f716b9756231
🤔 naumenkogs reviewed a pull request: "TxDownloadManager followups"
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2417664548)
ACK 8351562bec6081eda2a600bfe4edeb264a9dee0b
I haven't verified what happens if `time` goes below 0
(https://github.com/bitcoin/bitcoin/pull/31190#pullrequestreview-2417664548)
ACK 8351562bec6081eda2a600bfe4edeb264a9dee0b
I haven't verified what happens if `time` goes below 0
🚀 fanquake merged a pull request: "ci: Use clang-19 from apt.llvm.org"
(https://github.com/bitcoin/bitcoin/pull/30634)
(https://github.com/bitcoin/bitcoin/pull/30634)
👍 fanquake approved a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811#pullrequestreview-2417738480)
ACK 788c1324f3d840f7a39b8bc3537dcff26ca0b552
(https://github.com/bitcoin/bitcoin/pull/30811#pullrequestreview-2417738480)
ACK 788c1324f3d840f7a39b8bc3537dcff26ca0b552
✅ fanquake closed an issue: "Unify -logsourcelocations format"
(https://github.com/bitcoin/bitcoin/issues/30799)
(https://github.com/bitcoin/bitcoin/issues/30799)
🚀 fanquake merged a pull request: "build: Unify `-logsourcelocations` format"
(https://github.com/bitcoin/bitcoin/pull/30811)
(https://github.com/bitcoin/bitcoin/pull/30811)
💬 laanwj commented on pull request "net, init: derive default onion port if a user specified a -port":
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2459213686)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31223#issuecomment-2459213686)
Concept ACK
💬 dergoegge commented on pull request "TxDownloadManager followups":
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459317676)
oss-fuzz found a crash in txdownloadman_impl, perhaps worth including the fix here or in another followup:
```
$ FUZZ=txdownloadman_impl out/libfuzzer/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
src/test/fuzz/txdownloadman.cpp:290 CheckInvariants: Assertion `txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT' failed.
```
[clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt](https://github.com/user-att
...
(https://github.com/bitcoin/bitcoin/pull/31190#issuecomment-2459317676)
oss-fuzz found a crash in txdownloadman_impl, perhaps worth including the fix here or in another followup:
```
$ FUZZ=txdownloadman_impl out/libfuzzer/fuzz clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt
src/test/fuzz/txdownloadman.cpp:290 CheckInvariants: Assertion `txdownload_impl.m_txrequest.CountInFlight(peer) <= node::MAX_PEER_TX_REQUEST_IN_FLIGHT' failed.
```
[clusterfuzz-testcase-minimized-txdownloadman_impl-5180010463297536.txt](https://github.com/user-att
...
💬 dergoegge commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2459379945)
Perhaps relevant for the discussion around removing `testBlockValidity`: https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055947
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2459379945)
Perhaps relevant for the discussion around removing `testBlockValidity`: https://github.com/bitcoin/bitcoin/issues/31136#issuecomment-2453055947
💬 laanwj commented on pull request "tests: Handle BDB dynamic pagesize":
(https://github.com/bitcoin/bitcoin/pull/31222#discussion_r1830802184)
Might want to do some basic checks on the pagesize; eg
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0):
```
(https://github.com/bitcoin/bitcoin/pull/31222#discussion_r1830802184)
Might want to do some basic checks on the pagesize; eg
```python
MIN_PAGESIZE = 512
MAX_PAGESIZE = 65536
⋮
assert pagesize >= MIN_PAGESIZE and pagesize <= MAX_PAGESIZE and (pagesize & (pagesize-1) == 0):
```
💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459428224)
`99b1f88fe8...cf83f0c14c`: rebase due to conflicts and mock the sleeps
> > I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
> Sounds good to me.
Done. `CThreadInterrupt` can now be mocked in other tests as well. Thanks for the suggestion, @dergoegge!
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2459428224)
`99b1f88fe8...cf83f0c14c`: rebase due to conflicts and mock the sleeps
> > I guess it should be possible to mock CConnman::interruptNet so that its sleep_for() method returns immediately.
> Sounds good to me.
Done. `CThreadInterrupt` can now be mocked in other tests as well. Thanks for the suggestion, @dergoegge!
💬 dergoegge commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830823046)
Perhaps in a follow up we can make mocktime accurate to the millisecond and then advance it here to simulate an actual sleep.
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r1830823046)
Perhaps in a follow up we can make mocktime accurate to the millisecond and then advance it here to simulate an actual sleep.
👍 laanwj approved a pull request: "Update secp256k1 subtree to v0.6.0"
(https://github.com/bitcoin/bitcoin/pull/31216#pullrequestreview-2417975546)
ACK 97235c446e9986ecca09c2a4b78d6c6239853fdb97235c446e9986ecca09c2a4b78d6c6239853fdb
good to have the secp256k1_memclear commits
(https://github.com/bitcoin/bitcoin/pull/31216#pullrequestreview-2417975546)
ACK 97235c446e9986ecca09c2a4b78d6c6239853fdb97235c446e9986ecca09c2a4b78d6c6239853fdb
good to have the secp256k1_memclear commits
💬 fanquake commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2459458747)
Guix Build:
```bash
df8b274bbadabebfc2ec3d28591f6f66a983f2500779fb95016aaef8c728ad8d guix-build-97235c446e99/output/aarch64-linux-gnu/SHA256SUMS.part
26aeeb60bf02b1d6e1c072a5c42a3ec14165f208efb5f17992f1716c2db7b826 guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu-debug.tar.gz
043e16b13eed1ff5b3a1c4ce67309d595fffbdbbd0cd6800221a9eabb84a35eb guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu.tar.gz
3f2b606211c543d8
...
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2459458747)
Guix Build:
```bash
df8b274bbadabebfc2ec3d28591f6f66a983f2500779fb95016aaef8c728ad8d guix-build-97235c446e99/output/aarch64-linux-gnu/SHA256SUMS.part
26aeeb60bf02b1d6e1c072a5c42a3ec14165f208efb5f17992f1716c2db7b826 guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu-debug.tar.gz
043e16b13eed1ff5b3a1c4ce67309d595fffbdbbd0cd6800221a9eabb84a35eb guix-build-97235c446e99/output/aarch64-linux-gnu/bitcoin-97235c446e99-aarch64-linux-gnu.tar.gz
3f2b606211c543d8
...
💬 vasild commented on pull request "test: extend the SOCKS5 Python proxy to actually connect to a destination":
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1830836157)
I think this is worth a followup. If you agree, I would open one, or just leave it as it is? What to reword it to? "Ending handling of connection to ..."? Or log different messages based on `self.serv.keep_alive`?
(https://github.com/bitcoin/bitcoin/pull/29420#discussion_r1830836157)
I think this is worth a followup. If you agree, I would open one, or just leave it as it is? What to reword it to? "Ending handling of connection to ..."? Or log different messages based on `self.serv.keep_alive`?
📝 fanquake opened a pull request: "ci: `add second_deadlock_stack=1` to TSAN options"
(https://github.com/bitcoin/bitcoin/pull/31232)
This is mentioned in the developer notes, but isn't present in `TSAN_OPTIONS`, resulting in:
```bash
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508)
Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0
<snip>
Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message
```
Add it, for (potentially) more informative output, when failures occur. Checked that adding does output mor
...
(https://github.com/bitcoin/bitcoin/pull/31232)
This is mentioned in the developer notes, but isn't present in `TSAN_OPTIONS`, resulting in:
```bash
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=60508)
Cycle in lock order graph: M0 (0xffff98e02208) => M1 (0xffff98e0cbe8) => M2 (0xffff98e0cd98) => M0
<snip>
Hint: use TSAN_OPTIONS=second_deadlock_stack=1 to get more informative warning message
```
Add it, for (potentially) more informative output, when failures occur. Checked that adding does output mor
...
🚀 fanquake merged a pull request: "Update secp256k1 subtree to v0.6.0"
(https://github.com/bitcoin/bitcoin/pull/31216)
(https://github.com/bitcoin/bitcoin/pull/31216)
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2459516090)
The current implementation seems to be using the aggregate pubkey (before the tweaks) inside the key of the `PSBT_IN_MUSIG2_PUB_NONCE` (and I'd assume `PSBT_IN_MUSIG2_PARTIAL_SIGNATURE`, but I didn't reach there, yet); instead, [BIP-373](https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki) says that it must be _the key found in the script and not the aggregate public key that it was derived from, if it was derived from an aggregate key_. Therefore, I interpreted it as the taproot pubk
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2459516090)
The current implementation seems to be using the aggregate pubkey (before the tweaks) inside the key of the `PSBT_IN_MUSIG2_PUB_NONCE` (and I'd assume `PSBT_IN_MUSIG2_PARTIAL_SIGNATURE`, but I didn't reach there, yet); instead, [BIP-373](https://github.com/bitcoin/bips/blob/master/bip-0373.mediawiki) says that it must be _the key found in the script and not the aggregate public key that it was derived from, if it was derived from an aggregate key_. Therefore, I interpreted it as the taproot pubk
...
🤔 rkrux reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2417722869)
Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.
I've gone through only the src/policy/*, src/rpc/*, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2417722869)
Successful make and functional tests at 131bed19bdfc922328cad9781fa9122b6810a8fd.
I've gone through only the src/policy/*, src/rpc/*, src/validation.cpp yet. My comments are mostly around code structure. I plan to review the functional tests and src/tests/* very soon.
💬 rkrux commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830689563)
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where `IsDust()` is.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1830689563)
Nit: This function is independent of the ephemeral nature and is generic enough to lie outside this file, near where `IsDust()` is.