💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596947704)
might want to note this line is necessary for the reconsideration step to always happen

💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596977375)
is `send_message` + `sync_with_ping` always enough to resolve two generations of orphans to ensure we hit the mempool?

💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596981173)
```Suggestion
# 3. Honest peer announces the real child by txid (this isn't common but the node should
```

💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1596988905)
note for readers: If the child tx ended up being the same wtxid, and in that case the node would simply not add to the orphanage or request anything further.

📝 kevkevinpal reopened a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994)
Introduced in https://github.com/bitcoin/bitcoin/pull/23542 and released in version 23.0 there has been significant time since this change (2 years).

This should be removed as it is no longer relevant

💬 Sjors commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2104945955)
@vasild CI seems unhappy.

💬 kevkevinpal commented on pull request "doc: removed help text saying that peers may not connect automatically":
(https://github.com/bitcoin/bitcoin/pull/29994#issuecomment-2104948425)
> ACK [95897ff](https://github.com/bitcoin/bitcoin/commit/95897ff181c0757e445f0e066a2a590a0a0120d2)
>
> Why exactly was this closed?
>
> > I'm not sure this should be removed unless it has been observed to no longer be the case
>
> > According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher
>
> Those are (currently) 82.3% of the listening nodes. I think "unlikely to get incoming connections" is not true anymore and this sentence can be dropped.
>
>
...

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