💬 achow101 commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2104971798)
ACK 96378fe734e5fb6167eb20036d7170572a647edb

👍 kristapsk approved a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2050507810)
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2. According to https://bitnodes.io/dashboard/#user-agents stats, most nodes on the network are v23+.

🚀 achow101 merged a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252)

👍 theuni approved a pull request: "refactor: Model the bech32 charlimit as an Enum"
(https://github.com/bitcoin/bitcoin/pull/30047#pullrequestreview-2050530629)
Ok. I spent some time working on a correct-by-construction approach here with a new `Bech32Encoded` string type which knows its own size limit. Ultimately I don't think it's worth the complexity.

What's here doesn't seem ideal to me, but I can't come up with anything better and it's simple enough that it's obviously correct (without `NO_LIMIT` at least :).

utACK 696e0a5314296ca064192f580de949d5d9d8f9d4.

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2105011629)
Updated to add a move constructor to the KeyPair class. I noticed this was missing while trying to use the new class in #28201 (i.e. creating a std::vector of KeyPairs). @theuni assuming this was just missed, but if not curious if you have any objections to adding a move constructor on KeyPair?

💬 josibake commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2105014896)
Concept ACK

🤔 theuni reviewed a pull request: "crypto, refactor: add method for applying the taptweak"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2050565640)
Mind changing the dumb c-style casts to `reinterpret_cast` so it's clear that they can't be `static_cast`s ? Sorry, I know that's my code.

utACK after that.

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2105023865)
PR description needs an update too :)

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2105045459)
@theuni title, description, and dumb c-style casts updated!

🤔 jonatack reviewed a pull request: "refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition"
(https://github.com/bitcoin/bitcoin/pull/29086#pullrequestreview-2050593381)
Nice cleanup.

ACK cc67d33fdac45357b593b1faff3d1735e5fe91ba

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2105053007)
> and results in less data to download for light clients

This is really the crucial benefit: if the index does not implement cut-through, clients will download data that is not relevant to them.

Something that will also likely cut the index down by a lot is a "dust filter," i.e. prune transactions that have zero unspent taproot outputs _above the dust filter_. Something like 1000 sats, to give it some padding? The motivation here is a light client wont be able to spend these outputs anyway
...

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2105055399)
IMHO #29086 should be merged first, it's a good refactor, I will rebase then, so please review that one.

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-2105057683)
> Actually, since these don't change on their own... maybe a separate RPC altogether? (later extended to allow some changes?)

I'm not sure it's worth breaking compatibility here. Stuff like `minrelaytxfee` and `mempoolminfee` is checked by a lots of wallet software, Lightning nodes, etc.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2105076296)
> Something that will also likely cut the index down by a lot is a "dust filter,"

I have some preliminary numbers [here](https://github.com/setavenger/BIP0352-light-client-specification/?tab=readme-ov-file#dust-limits). They are in the same ballpark as another analysis. Next step would be to see how this reduced UTXO set will actually affect the number of tweaks. Specifically, I'd like to find out to which degree we can actually reduce the set of tweaks based on how dust UTXOs are clustered p
...

👍 theuni approved a pull request: "refactor: Remove unused code from `subprocess.h` header"
(https://github.com/bitcoin/bitcoin/pull/30081#pullrequestreview-2050629489)
Easy code review ACK 5a11d3023f7d0cde777f3496c0f3aa381823d749 since it's all removals :)

I assume since c-i is green that this code is all unneeded, but I can't attest to the specifics.

💬 brunoerg commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2105082713)
From CI:
```
291/305 -
rpc_help.py
failed, Duration: 1 s
stdout:
2024-05-10T15:06:20.992000Z TestFramework (INFO): PRNG seed is: 4539310303736745094
2024-05-10T15:06:20.998000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20240510_150004/rpc_help_7
2024-05-10T15:06:21.349000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/te
...

💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1597085098)
-B isn't sufficient here?

👍 theuni approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2050642049)
ACK bdc2a656c2d2a61d226fde1d1fd4e79664106e18

👍 theuni approved a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048#pullrequestreview-2050650940)
Code review 57a06646952fed98c1c281f02fe58a0758a8ed5a. I didn't verify with sage.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2105116651)
Alright, let's see how it goes with dust limit 1000.