💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449289058)
I think this field is redundant. This will always be `True` if `v2_state` is set, and `False` otherwise, so we could drop it and just check against whether v2_state is `None`.

If you think that's too verbose, you could also create a helper method:

```python
def supports_v2_p2p(self):
return self.v2_state is not None
```

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449336950)
nit: space missing after responder, also I guess add "the" before to match the previous suggestion if you happen to take it

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449352220)
Is this necessary?

`self.v2_state.received_prefix` is initialized to `b""`, whose length is zero. Therefore, and given you are only using this as offset for `self.recvbuf`, I think you could just dump the variable and do (in line 266):

```
response = self.v2_state.complete_handshake(BytesIO(self.recvbuf[len(self.v2_state.received_prefix):]))
```

Or define the variable if you want a shorter call but just right before using it, without having to set it to zero first.

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450888252)
nit: connection (for consistency with the rest)

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1450866270)
I'm really struggling to understand what is going on here based on these comments (and the ones bellow at L98-99).

To my understanding, what we are trying to test here is: given a peer of `node0` (`peer6`) that provides some blocks to it (decoy on the first iteration and regular on the second) if after connecting `node0` and `node1`, they do end up sharing the same tip. However, I feel the comments are hard to understand

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449337692)
nit: P2PConnection is "the" initiator?

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449340059)
nit: caps in "the"

💬 chrisguida commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889953978)
Concept ACK

This PR seems to be the minimal feature set to get:
- LN-Symmetry,
- much better PTLCs, and
- all of the powerful UTXO-sharing protocols that CTV enables

Regarding LN-Symmetry, this update will FINALLY enable symmetric channel updates and eliminate the risk of losing all your money due to restoring an old backup. It also makes on-chain resolution in the case of uncooperative channel closes much less painful, and makes watchtowers much more efficient. There are still some mem
...

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1889982713)
ready for review

cc @glozow @ismaelsadeeq @achow101

🤔 jonatack reviewed a pull request: "doc, test: test and explain service flag handling"
(https://github.com/bitcoin/bitcoin/pull/29213#pullrequestreview-1819009947)
ACK d536e5a6325d1885224f36debdcc5245b94efe8a

💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450936649)
It seems to me this might be helpful and the message names are unlikely to change.

```diff
// Overwrites potentially existing services. In contrast to this,
- // unvalidated services received via gossip relay are only
- // ever added but cannot replace existing ones.
+ // unvalidated services received via gossip relay in ADDR/ADDRV2
+ // messages are only ever added but do not replace existing ones.
```

💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450905336)
Perhaps add a "see `AddSingle()`" mention somewhere in here, as the logic referred to here is in that method called from this one (`Add/Add_`).

💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1450951378)
```suggestion
// Adding service flags even works when the addr is in Tried; see `AddSingle()`
```

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890013810)
Gawd, SFFO is such pain in the neck.

💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450962291)
> Again, no opinion on this. I think either solution is fine. PRs welcome, I guess?

Thanks, I started playing around with a change that should make it easier to set the category without being verbose, and also start relying less on a global logging instance, which should be useful for kernel applications and tests: 262923c5b37f62a925a2c5611318855009bfb241. Will see how this looks and probably open a PR.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1890125580)
Fixed bug discovered with the optimality fuzz test: [opt: Skip over barren combinations of tiny UTXOs](https://github.com/bitcoin/bitcoin/pull/27877/commits/80bcee5c09fb68366645d6be62fdeab4bb3ec6f3), incorrectly was set to cut, but this could be premature in case that the last selected UTXO was heavier than later UTXOs in the pool.

📝 achow101 opened a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243)
AttachChain will create the chain notifications handler which contains a reference to the wallet's shared_ptr. If AttachChain fails, the wallet needs to be unloaded, and this is expected to happen with its custom deleter ReleaseWallet. However, if the chain notifications handler is still set, then the shared_ptr is still referenced by something, so the wallet is never actually released.

💬 achow101 commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1890229823)
This actually revealed quite a subtle bug in the way that we handle cleaning up after an `AttachChain` failure. I've opened a fix in #29243

💬 achow101 commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1890230397)
I believe I've found the root cause of the test failure, fix in #29243

💬 jamesob commented on pull request "wallet: Reset chain notifications handler if AttachChain fails":
(https://github.com/bitcoin/bitcoin/pull/29243#issuecomment-1890246257)
Huh, any clue as to why this only becomes an issue on Windows?