💬 jonatack commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1880180224)
In commit 0ac5cf9141441ea539d2f345d0d516d24a2f77bd

```
$ ./src/test/test_bitcoin -t transaction_tests
Running 7 test cases...
test/transaction_tests.cpp:192: error: in "transaction_tests/tx_valid": mapFlagNames is missing a script verification flag
```

💬 hsjoberg commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1880182827)
Concept ACK; this would make pretty much all sides happy.

💬 bordalix commented on pull request "RPC: add new `listmempooltransactions`":
(https://github.com/bitcoin/bitcoin/pull/29016#issuecomment-1880184679)
Concept ACK

📝 jonatack opened a pull request: "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption"
(https://github.com/bitcoin/bitcoin/pull/29200)
A Bitcoin Core node may only connect to a peer destination via I2P if both sides have sessions with the same encryption type. Encryption type is a property of the session, not the destination. Sessions may support multiple encryption types.

As Bitcoin Core is not currently setting the encryption type when creating I2P sessions, it uses the older default, ElGamal (type 0).

This pull updates our I2P session creation to use both ECIES-X25519 and ElGamal (types 4 and 0, respectively). This a
...

💬 zzzi2p commented on pull request "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption":
(https://github.com/bitcoin/bitcoin/pull/29200#issuecomment-1880235978)
ACK 9d728916b27e18efc6f8839770ed5ec14789fc08

👍 theStack approved a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1807916160)
ACK fb5bfed26a564014b83ccfc96ff00b630930fc61

Reviewed the code and verified the intended behavior for manual connections (`-connect`, `-addnode`) and addrfetch connections (`-seednode`) by specifying both a p2p-v2-supporting peer and a p2p-v1 peer in different bitcoind runs. For the v1 peer, the first (v2) connection attempt fails, and a successful v1 reconnect happens after, just as expected.

👍 kristapsk approved a pull request: "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption"
(https://github.com/bitcoin/bitcoin/pull/29200#pullrequestreview-1807916410)
cr utACK 9d728916b27e18efc6f8839770ed5ec14789fc08

⚠️ Kongpc256 opened an issue: "The"
(https://github.com/bitcoin/bitcoin/issues/29201)

💬 ajtowns commented on pull request "test: Add test case for spending bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1444113496)
> 2/3 may require mining them into blocks directly if the outputs are non-standard to make

`self.generateblock(node, address, [tx.serialize().hex()])` is already mining the tx-to-be-spent directly into a block.

2-of-3 shouldn't make any difference vs 1-of-3 or even 1-of-1 afaics. If the tx-to-be-spent is already confirmed, there shouldn't be any problem having a spend of a 20-of-20 checkmultisig in the mempool, so I don't think n/m=4 is actually an edge case. I think a 0-of-0 checkmultisig
...

💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1880277106)
> I believe most of them use libbitcoinconsensus for testing script execution, with the only exception of `floresta`? While I found it beneficial for cross-testing complex scripts, the absence of core policy rule checking became a notable drawback.

Would something along the lines of https://github.com/ajtowns/bitcoin/commits/202309-evalscript/ be an interesting alternative?

```sh
$ ./bitcoin-util -script_flags=STANDARD evalscript '515193'
{
"script": {
"asm": "1 1 OP_ADD",
"
...

👍 theStack approved a pull request: "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py"
(https://github.com/bitcoin/bitcoin/pull/29067#pullrequestreview-1807941957)
Code-review ACK fa3b4dbf45b2b7ff1bdac6f68cf23275102cc775

👍 kristapsk approved a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1807944717)
ACK fb5bfed26a564014b83ccfc96ff00b630930fc61

💬 jonatack commented on pull request "net: create I2P sessions using both ECIES-X25519 and ElGamal encryption":
(https://github.com/bitcoin/bitcoin/pull/29200#issuecomment-1880308961)

💬 ajtowns commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1880309647)
Not an objection, but this feels backwards to me: presumably the user has manually listed those peers as addnode/connect because it's important that their node be connected to those peers, but if there are any bugs in the "v2 didn't work, retry as v1" logic, it's exactly those peers that will be inaccessible. But if we're confident there aren't bugs in that logic, why not just apply it to all peers? The "if you don't like it, just set -v2transport=0" approach to avoiding bugs/risk works just as
...

📝 jonatack opened a pull request: "Remove no-longer-needed NOLINTNEXTLINE since C++20 upgrade"
(https://github.com/bitcoin/bitcoin/pull/29202)
This was a temporary workaround for C++17 in https://github.com/bitcoin/bitcoin/pull/28583 and should no longer be needed for the CI tidy task now that the project has moved to C++20.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1880319951)
> why not just apply it to all peers

Do you mean why not apply it to automatic peers as well per default (and ignoring the information from the service flag)? I thought the idea was to save one disconnection / reconnection, and the time spent doing these when we probably know better. But as I wrote in https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1854197471, I think that we depend on the reconnection mechanism for automatic connection as well, because anyone can add a wrong "v2"
...

💬 kevkevinpal commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1880357066)
Concept ACK [2422b90](https://github.com/bitcoin/bitcoin/pull/29156/commits/2422b90978f4ea13ee49954598dc8aef841e36df)

I've done a little code review and so far it is looking good, I think it would be cool if instead of always using the pubkeys in the same order when decaying we could randomly use different signers, I just quickly tested it worked the other way around with this diff (not sure how you feel about adding this randomness)

```
for m in range(self.M):
-
...

✅ jonatack closed a pull request: "refactor: use push_back in TaprootBuilder::Add(), rm NOLINTNEXTLINE and comment"
(https://github.com/bitcoin/bitcoin/pull/29202)

💬 totient commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1880361556)
> > It's meant to limit extra data in transactions. OP_RETURN was supposed to be the only tolerated way to do that.
>
> Can you add any references for this? because the GitHub and git history very clearly contradicts your statements ([#29187 (comment)](https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1878972398)).

"There was been some confusion and misunderstanding in the community, regarding the OP_RETURN feature in 0.9 and data in the blockchain. This change is not an endorse
...

🤔 BrandonOdiwuor reviewed a pull request: "refactor(tidy): Use C++20 contains method"
(https://github.com/bitcoin/bitcoin/pull/29191#pullrequestreview-1808113803)
Concept ACK using ::contains(Key& key) (C++20) to check for elements