👍 kristapsk approved a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040#pullrequestreview-1518168856)
utACK bea9fc2600635020fd28ec7a6613c92a6f349a86

💬 MarcoFalke commented on pull request "test: bugfix, synchronize indexes synchronously":
(https://github.com/bitcoin/bitcoin/pull/28026#issuecomment-1624951013)
Does your feedback from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703 also apply here? I think you'll also have to check for it being synced?

```
Assert(index.GetSummary().synced);

👍 hebasto approved a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040#pullrequestreview-1518273732)
ACK bea9fc2600635020fd28ec7a6613c92a6f349a86, I have reviewed the code and it looks OK.

👍 dergoegge approved a pull request: "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization"
(https://github.com/bitcoin/bitcoin/pull/28012#pullrequestreview-1518335145)
Code review ACK fac6af16f4a254458b8cb3522317422b40362f8d

👍 PRADACANDI18 approved a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040#pullrequestreview-1518344833)
Looks good

🚀 fanquake merged a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040)

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255475955)
Question: what is the purpose of using both `getblocktemplate` and `generate` for this test?

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1254217360)
nit: given the numerous minimum feerates that exist, imo it's good to use specific names to disambiguate. For example the config option name `-blockmintxfee` vs `-minrelaytxfee`, or the RPC result name "mempoolminfee" vs "minrelaytxfee" vs "incrementalrelayfee"

In this case
```suggestion
# submit one tx with exactly the blockmintxfee rate, and one slightly below (if possible)
```

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1254664539)
Since the below-blockmintxfee transactions don't get mined, they hang out in the mempool across iterations of the for loop and after the test. I think it'd be cleaner to clear it across restarts, as it'll help if another subtest added after this one
```suggestion
self.restart_node(0, extra_args=[blockmintxfee_parameter, '-minrelaytxfee=0', '-persistmempool=0'])
```

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1254268972)
For the ==0 case, you could add some coverage for the fact that modified fees are used?
```suggestion
tx_below_min_feerate = self.wallet.send_self_transfer(from_node=node, fee_rate=blockmintxfee_btc_kvb)
node.prioritisetransaction(tx_modified_below_min["txid"], 0, -1)

```

💬 glozow commented on pull request "test: miner: add coverage for `-blockmintxfee` setting":
(https://github.com/bitcoin/bitcoin/pull/27620#discussion_r1255490340)
Are you sure `=0` means disabled? AFAICT it is still enforced, but you can only hit it using `prioritisetransaction` to make the fee negative. "Disabled" suggests to me that the block assembler would include packages at any feerate.

🚀 fanquake merged a pull request: "test: Restore unlimited timeout in IndexWaitSynced"
(https://github.com/bitcoin/bitcoin/pull/28036)

👍 hebasto approved a pull request: "wallet: don't include bdb files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28039#pullrequestreview-1518398328)
Approach ACK 2d09d0f50408f9c522b9efa4f386072502c7b3d1.

Suggesting to adjust our IWYU mapping file:
```diff
--- a/contrib/devtools/iwyu/bitcoin.core.imp
+++ b/contrib/devtools/iwyu/bitcoin.core.imp
@@ -4,4 +4,7 @@
{ include: [ "<bits/termios-struct.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/termios-tcflow.h>", private, "<termios.h>", public ] },
{ include: [ "<bits/chrono.h>", private, "<chrono>", public ] },
+
+ # Berkeley DB
+ { include: [ "<db.h>", pri
...

💬 MarcoFalke commented on pull request "fuzz: Generate rpc fuzz targets individually":
(https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1625108039)
Is this rfm?

🚀 fanquake merged a pull request: "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization"
(https://github.com/bitcoin/bitcoin/pull/28012)

💬 hebasto commented on pull request "bugfix: Make `CCheckQueue` RAII-styled":
(https://github.com/bitcoin/bitcoin/pull/26762#issuecomment-1625154134)
Rebased e89273e80765219ff545ee5c26a257aef7d69f9c -> b7ebc999ee373f0429939ac0bdb8e38fb37404aa ([pr26762.11](https://github.com/hebasto/bitcoin/commits/pr26762.11) -> [pr26762.12](https://github.com/hebasto/bitcoin/commits/pr26762.12)) due to the conflict with #27861.

💬 willcl-ark commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1625181541)
> > This has the effect on this PR that many of the every~5 minute network specific connections are wasted on ipv6 attempts.
>
> completely related, thanks for sharing! this makes sense based on the current implementation. the original proposal had special case logic to prevent this situation, but the direction of review has led to deciding this is an acceptable tradeoff. incase helpful, [#27213 (comment)](https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1577424366) provides a recap
...

🚀 fanquake merged a pull request: "fuzz: Generate rpc fuzz targets individually"
(https://github.com/bitcoin/bitcoin/pull/28015)

💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255585328)
Is adding the txid still necessary though?

IIUC, the purpose of adding this transaction's txid to `inventory_known_filter` is to save a step if this transaction has an unconfirmed child (if it already existed in this filter, then we wouldn't add it to the `recently_announced` filter). The PR is deleting those lines from `ProcessGetData`, and that's fine since an unconfirmed parent's sequence number should before that of its child.
But I'm not able to see another reason why we should add the
...

💬 glozow commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1255631372)
```suggestion
/** Target number of transaction inventory items to send per transmission. */
```