💬 achow101 commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1624433876)
Is the current ChaCha20 implementation used for anything that needs backwards compatibility?

💬 sipa commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#issuecomment-1624469014)
@achow101 Good question!

It's used by FastRandomContext and MuHash. I haven't investigated whether there are any uses of FastRandomContext that could generate more than 256 GiB of data, but even if there are there it's possible to at least use the new seeking style (the current implementation, when using the 96/32 split, when the block counter overflows, just increments the nonce, which is exactly what we'd want for an RNG anyway).

So no - I think all existing uses could be converted to us
...

💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1255193877)
Okay so I’ve been looking into the code, and yes to my understanding there is _no_ processing penalty at `GETPKGTXNS` reception in `ProcessMessage` than our child has 1 or N parents, we’re calling `FindTxForGetdata` to compose the response `PKGTXNS`. This invalidate this specific source of concern.

> we have rate-limiting based on the way the parents are fetched (i.e individiually rather than in batch) the equivalent package of A+E will propagate better.

I’ll maintain there is still an int
...

💬 ariard commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1255197289)
Yes adding a mention in BIP331 than a p2p packages can support multiple versions of policy regimes e.g nversion=3.

For flexibility towards the node operators in their resource management (especially for DoS protection on low-performance hosts), I think we should have a `acceptnversion=3`setting, that way a node operator can opted in package relay though not in the most computationally expensive type of policy regimes. The utility of such setting is theoretical as long as we have one policy r
...

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