💬 ajtowns commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1846204744)
> > What does this mean? I'm seeing file sizes of 448MB (with fuzzing enabled) and 146MB (with a normal build) for test/fuzz/fuzz.
>
> If you build this pull request you should see a binary per fuzz harness in `src/test/fuzz` (e.g. `src/test/fuzz/process_message`), as well as the usual fuzz binary `src/test/fuzz/fuzz`. If you sum up the size of the individual per harness binaries (assuming you compile with LTO) you should end up with roughly 3.6GB (maybe this depends on compiler version etc.)
...

⚠️ prusnak opened an issue: "DB_RUNRECOVERY: Fatal error, run database recovery"
(https://github.com/bitcoin/bitcoin/issues/29026)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

We are experiencing the build failures in NixPkgs both on x86_64-darwin and aarch64-darwin:

```
> 2023-12-07T14:45:33.311681Z [test] [wallet/bdb.cpp:189] [Open] BerkeleyEnvironment::Open: Error -30974 opening database environment: DB_RUNRECOVERY: Fatal error, run database recovery
> make[3]: *** [Makefile:22524: wallet/test/wallet_tests.cpp.test] Error 1
```

The full
...

🤔 ishaanam reviewed a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1771245748)
ACK 1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

💬 1440000bytes commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1846220961)
Approach NACK

1. `DatacarrierBytes()` will need an update every few weeks as explained by @Sjors in https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1844981799 and could affect some other use cases.
2. `datacarriersize` is used for OP_RETURN and should not be mixed with other things. We should remove default limits for OP_RETURN because it does not affect UTXO set and there is no witness discount involved.
3. I do not see any issues with inscriptions. Neither they are spam nor an
...

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1846256921)
Updated 0e647454b49274ee82978c06b606f3fd9c02b779 -> 778c0214d25e5a012c61251d592257ae19805255 ([`pr/rmutil.1`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.1) -> [`pr/rmutil.2`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.1..pr/rmutil.2)) fixing CI errors from forgitting to add new files to git

re: https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1844180519

> > Motivation for this change is t
...

💬 ajtowns commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1846286294)
> > > Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications.
> >
> > Is the idea here that `util` should be for generic code, and that these bits and pieces (like fees and error) are specific bitcoin features that are only relevant to wallet/p2p, so aren't generic?
>
> I do think that's generally a good thing to do, but it's not really the goal of this PR
...

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419736521)
nit: as the `v1_prefix` only depends on `self.net` and never changes, could set it once before the loop (or even in the constructor and store it as member, if we'd ever need it somewhere else), rather than repeatedly on each iteration.

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1419733098)
If the length returned by `v2_receive_packet(...)` is -1 due to a MAC tag mismatch, `processed_length` is decreased by one here, which is very likely unintended. Currently this is not a problem (the call-site in the later introduced method `P2PConnection.v2_handshake` throws an exception then anyways and doesn't do anything with the returned length), but it's probably better if the `processed_length` increase happens after the length-checks and returns a few lines below.

💬 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