💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#discussion_r1451789060)
Decision was somewhat arbitrary on my part. ECDSA doesn't really add anything in Tapscrpit where Schnorr adds ability to use well specified MPC protocols like MuSig2 and FROST in legacy, so I settled on adding Schnorr for legacy but not adding ECDSA for Tapscript.

In Tapscript, I wonder if we'll eventually want to fork in 33-byte 02/03 keys as x-y Schnorr, so I was a bit hesitant to consume those prefixes now.

💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#discussion_r1451789140)
ECDSA requires 32-byte message. OP_SHA256 ahead would enable scripts to handle other data sizes. Making that explicit seems like the better option for ECDSA to enable shorter scripts when pre-hashing is not needed, but also allow pre-hashed data when needed.

💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1891047926)
Thanks for the comments @benthecarman !

💬 hebasto commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1891059064)
I've rerun the "Win64 native" CI job just to assure that the "wallet_assumeutxo.py --descriptors" test passes again.

💬 hebasto commented on issue "ubsan: misaligned-pointer-use in crc32c/src/crc32c_arm64.cc":
(https://github.com/bitcoin/bitcoin/issues/29178#issuecomment-1891063873)
A possible solution suggested in https://github.com/bitcoin-core/crc32c-subtree/pull/6.

💬 hebasto commented on pull request "Revert "build: Fix regression in "ARMv8 CRC32 intrinsics" test"":
(https://github.com/bitcoin/bitcoin/pull/29226#issuecomment-1891064076)
An alternative approach suggested in https://github.com/bitcoin-core/crc32c-subtree/pull/6.

💬 hebasto commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891065737)
> Seems confusing to have a test runner that calls another runner (`all-lint.py`), which calls a subset of the lint tests.

However, it is quite convenient to run `./test/lint/all-lint.py` locally, no?

💬 alfonsoromanz commented on pull request "Fix typos":
(https://github.com/bitcoin/bitcoin/pull/29246#issuecomment-1891073266)
ACK a1b1798b1652e93caf66e5e1c8353bcd7951f1c4

📝 0xBEEFCAF3 opened a pull request: "Reenable OP_CAT "
(https://github.com/bitcoin/bitcoin/pull/29247)
Renabling OP_CAT, as per the [draft BIP](https://github.com/bitcoin/bips/pull/1525)

This MR involves reinstating the legacy opcode by replacing OP_SUCCESS126. Currently, there are no proposed activation parameters.

### Relevant Links
[Btcd implementation](https://github.com/btcsuite/btcd/pull/2095)
[Signet MR](https://github.com/bitcoin-inquisition/bitcoin/pull/39)
[Mailing list post](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-October/022049.html)

👍 andrewtoth approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1820309688)
ACK 6c603490023317a84c7c96ef4b64f4f96ea03d1f modulo nits

For `peerman_tests.cpp`, we should prefer list initialization and constness for variables when possible.

For commit message of `net: move state dependent peer services flags` (b934bfd327f68d39aebb2d9ab52b39b7cd29525d),
the last sentence could be:
```
Additionally, this encapsulation enables us
to customize the connections decision-making process based on
new users' configurations in the future.
```

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1451813298)
nit: `int64_t best_block_time{};`

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1451813562)
nit: prefer list initialization, could also make this `constexpr` since we're modifying it.
`constexpr ServiceFlags requiredServiceBits{SeedsServiceFlags()};`

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1451815522)
nit: Could be one line
```
// Limited peers are desirable when we are close to the tip.
if ((services & NODE_NETWORK_LIMITED) && ApproximateBestBlockDepth() < NODE_NETWORK_LIMITED_MIN_BLOCKS) {
```

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1451815870)
nit: `2024`

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1451818095)
nit
```
// By now, we tested that 'CheckForStaleTipAndEvictPeers' can update the connections desirable services flags.
```

💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1451818200)
nit
```
// Verify a block close to the tip enables limited peers connections
```

👍 yahiheb approved a pull request: "Fix typos"
(https://github.com/bitcoin/bitcoin/pull/29246#pullrequestreview-1820328168)
ACK [a1b1798](https://github.com/bitcoin/bitcoin/pull/29246/commits/a1b1798b1652e93caf66e5e1c8353bcd7951f1c4)

💬 Farnoosh85 commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29110#issuecomment-1891204537)
0xE7cF1C52C9f179B855D7a31eA79997882269057A send your bnb bep20 address my

💬 torkelrogstad commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-1891514722)
@ismaelsadeeq I fixed the tests per your comments. Note that I pass the arguments through both the `arg_` prefixed and non-prefixed versions. Figured we might as well validate that there's no behavior changes between passing as positional arguments or through `options`.

Also added a new commits with tests that validates that `estimate_mode` is passed together with a confirmation target.

💬 ajtowns commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#discussion_r1452031664)
Might be better if this information were also exposed to the gui? Perhaps something along the lines of:

```c++
auto& cm = active_chainstate.m_chainman;
if (!cm.IsInitialBlockDownload()) {
cm.GetNotifications().progress("Loading mempool...", percentage_done, false);
}
```

would make sense? (completely untested)