💬 ariard commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1624619100)
From a reviewing standpoint on #27742, I think it would benefit if the release aim for current package version (BIP331 + nversion=3 policy regime) is explicitly bounded to Lightning time-sensitive confirmation flows, especially in matters of DoS protection:
- broadcast of revoked transaction output spend
- broadcast of a commitment transaction with pending HTLC outputs and its associated second-stage transactions

I think this covers the most sensitive Lightning flows impacted by the pinning
...

👍 MarcoFalke approved a pull request: "wallet: don't include bdb files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28039#pullrequestreview-1517976711)
lgtm

💬 MarcoFalke commented on pull request "wallet: don't include bdb files from our headers":
(https://github.com/bitcoin/bitcoin/pull/28039#discussion_r1255284707)
nit in c217c542f0fd9c741b79a91419247bd44440e6d9: I think you forgot `static` or `namespace`?

👍 MarcoFalke approved a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040#pullrequestreview-1517997528)
lgtm. I wonder why iwyu can't transform those two symbols into forward decls. Output on master:

```
The full include-list for wallet/sqlite.h:
...
#include <sqlite3.h> // for sqlite3_stmt, sqlite3
...

👍 TheCharlatan approved a pull request: "wallet: sqlite: don't include sqlite files from our headers"
(https://github.com/bitcoin/bitcoin/pull/28040#pullrequestreview-1518158272)
Nice, ACK bea9fc2600635020fd28ec7a6613c92a6f349a86

👍 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)