💬 achow101 commented on pull request "doc: add historical release notes for 26.0":
(https://github.com/bitcoin/bitcoin/pull/29023#issuecomment-1846005450)
ACK ca5937553b4b4dde53995d0b66e30150401023eb

🚀 achow101 merged a pull request: "doc: add historical release notes for 26.0"
(https://github.com/bitcoin/bitcoin/pull/29023)

💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1846105723)
Force-pushed addressing ASMap health check. cc: @fjahr

💬 Retropex commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1846169419)
I understand your argument.

However, now `OP_RETURN`, baremultisig, and other flaws such as `OP_0` `OP_IF`, are used to spam the chain. So what do you think we should do? Let them spam the chain by what we can't give them a toy on which to spam?

🤔 achow101 reviewed a pull request: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944#pullrequestreview-1771199234)
ACK f348eadadd6099e7763f04eb3d42cfd8bba33146

💬 achow101 commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1419702642)
In f348eadadd6099e7763f04eb3d42cfd8bba33146 "test: test sendall and send do anti-fee-sniping"

nit: `verbose` here too.

💬 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.