💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419779955)
Is this early check needed? I think `v2_receive_packet` already takes care of these "only part of packet is received" cases and indicates this with a corresponding return code.

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419777979)
nit: could check that `msg` has the required minimum length (13) and raise an error if it doesn't, to avoid a potential index out-of-bounds exception (even though this is more theoretical now, as bitcoind wouldn't send such an invalid message).

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1846306653)
Thanks, @furszy, I reworked your commit a bit, that was a big help! It did mean dropping support for `fuzz`, `bench_bitcoin`, and `test_bitcoin-qt` but this change really only benefits `test_bitcoin` (in my view). This PR is ready to review. Note the updated PR title and description.

👍 theStack approved a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1771339469)
re-ACK 1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

🤔 furszy reviewed a pull request: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564#pullrequestreview-1771340722)
Cool, great you liked it. Will review it soon.
Ended up connecting this work to https://github.com/bitcoin/bitcoin/pull/28955#pullrequestreview-1766044359.

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419807681)
Great catch, thanks!. Fixed.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1846332843)
> Does that mean that things should only go in util if they're going to be used by the kernel library then?

No, I don't think so, and I don't think much else is going to move after this PR. I think only code that _shouldn't_ be used by the kernel or kernel applications should be moved out of util, not just code that _isn't_ used by the kernel. I agree just moving any code that isn't used by the kernel would be a bad approach, and wrote basically the same comment as you in https://github.com/b
...

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#discussion_r1419807733)
Done

💬 furszy commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1846340360)
Updated per feedback. Thanks.
Applied @murchandamus suggestion of adding inputs fee to each UTXO rather than deduct them from the target. The latter one was breaking the equivalence of the input sets.

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1419817421)
In 5dd89adbc:
Can drop this now. Custom test data directories are not deleted anymore.

This is good for running test on a different storage location. Not only on the OS temporary files directory.

👍 andrewtoth approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1771466506)
ACK afc29b15e262b4ff402d479ec77ab8507552bcbc

👍 BrandonOdiwuor approved a pull request: "wallet: batch all individual spkms setup db writes in a single db txn"
(https://github.com/bitcoin/bitcoin/pull/28894#pullrequestreview-1771473291)
ACK f05302427386fe63f4929a7198652cb1e4ab3bcc

✅ toolsopen closed an issue: "sendrawtransaction takes too long"
(https://github.com/bitcoin/bitcoin/issues/28745)

💬 martinus commented on pull request "mempool: Don't sort in entryAll":
(https://github.com/bitcoin/bitcoin/pull/29019#discussion_r1420051555)
you could just write
```cpp
AssertLockHeld(cs);
return {mapTx.begin(), mapTx.end()};
```

💬 theStack commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1420130336)
You're right, in this case there shouldn't be any previous disconnect issues and hence the current connection-count approach would also work. I still decided to also use `wait_for_new_peer` here for consistency reasons and and it's less code and more readable.

💬 theStack commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#issuecomment-1846804212)
Thanks for the reviews! I'm leaving the unrelated nits (https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416885349 and https://github.com/bitcoin/bitcoin/pull/29006#discussion_r1416891272) for a follow-up.

💬 maflcko commented on pull request "test: fix v2 transport intermittent test failure (#29002)":
(https://github.com/bitcoin/bitcoin/pull/29006#issuecomment-1846851146)
Ok, lgtm ACK 00e0658e77f66103ebdeb29def99dc9f937c049d

💬 fanquake commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1846855849)
Ok. I think this was broken in 17.x, in this change https://github.com/llvm/llvm-project/commit/1f173a0653e7f0c3800033edfa16ffe4c35cde85. You should be able to build 16.x. i.e `guix build --target=riscv64-linux-gnu llvm@16.0.6`. This is something we should be able to fixup/patch around.

💬 maflcko commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420176846)
Maybe submit this (bugfix? or change?) in a separate pull?

👍 theStack approved a pull request: "wallet: batch all individual spkms setup db writes in a single db txn"
(https://github.com/bitcoin/bitcoin/pull/28894#pullrequestreview-1771968085)
Code-review ACK f05302427386fe63f4929a7198652cb1e4ab3bcc