💬 Eunovo commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1464304643)
@josibake p2sh-p2pkh or p2sh-p2pk are not addressed in the BIP. While I understand they are not commonly used scripts but `Solver(redeem_script, solutions)` can still detect p2pkh or p2pk in the redeem_script. Is there anything against adding support for these scripts? assuming that we ignore non-standard or malleated p2pkh in the redeem script

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464318846)
(originally because `on_connection_send_msg` would be end up being unused in v2 connection anyways and wanted to keep the `on_connection_send_msg` send during the v1 reconnection phase.)

interesting question! you're talking about a less common scenario where initiator node is preparing for v2 connection thinking responder node is v2 but v1 version message from responder node gets sent before v2 ellswift bytes from initiator node. if this happens the initiator node wouldn't attempt a reconnect
...

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464356180)
hmm other than the ellswift bytes, `v2_handshake()` also needs to read garbage bytes (could be received as separate chunks with size < 16 bytes) and other things like garbage terminator, decoy messages and version packet. i'm leaning towards keeping the interface generic and processing as we receive bytes.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464363550)
oh that way! the last test was for v1 behaviour and peer7 needed a v1 `TestNode` while all the others needed a v2 `TestNode` which is why it was kept separately in the end.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464367456)
good catch! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464367790)
done. liked the simpler language.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464368898)
good find! replaced it with an `else` pathway in `data_received()`. this was kept to not perform parsing/deserialising of P2P messages inside `_on_data()` in case `v2_handshake()` wasn't over.

done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464371637)
added more comments. meant it as the [ASCII message type](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#v2-bitcoin-p2p-message-structure) like in v1.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464372562)
true! removed.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464372887)
nice! done.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464375410)
right, not much of a difference - leaving it as a follow up if desired.

💬 maflcko commented on pull request "validation: improve checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#discussion_r1464467510)
```suggestion
// Transaction may be completely unset - happens if only the header was accepted but the block or it's ancestors haven't been downloaded.
```

nit: The block itself may have been processed (ProcessNewBlock is called). Maybe clarify that you meant that this block and all previous ones have been processed or downloaded to storage?

💬 maflcko commented on pull request "validation: improve checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#discussion_r1464471710)
```suggestion
// nChainTx may be unset, but nTx set (if a block has been accepted, but one of its predecessors hasn't been processed yet)
|| (pindex->nChainTx == 0 && pindex->nTx >=1 && prev_chain_tx == 0 && pindex->pprev)
```

nit: Missing nTx check, according to comment?

💬 maflcko commented on issue "Build error on macOS, tag v26.0 builds fine":
(https://github.com/bitcoin/bitcoin/issues/29289#issuecomment-1907608162)
> I think the conclusion is that I'm on clang 13 and need to upgrade. I'll close this issue (I don't have the will power to upgrade Xcode/clang on that machine right now and verify, but if it doesn't resolve the issue when I get to this I'll report back)

I think you are on clang 12 (for some reason apple calls clang 12 clang1300), according to https://en.wikipedia.org/wiki/Xcode#Xcode_11.0_-_14.x_%28since_SwiftUI_framework%29

But yeah, let us know if this is still an issue after upgrading
...

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464497339)
I think it deserves a comment :)

💬 maflcko commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1907627949)
> > a new label can be added, if a dedicated "category" is needed
>
> I think this would be great. Ideally there would just be some link you could go to quickly see recent known CI failures and what the expected error messages look like.

Ok, I've recycled the "CI failed" label for this. See https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue+is%3Aopen+label%3A%22CI+failed%22

There are 13 issues right now. (14 if someone creates an issue for the one mentioned in this thread)

If the
...

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1464515415)
The goal of this class is mostly to test the tests, right? Commenting that more clearly could help.

💬 S3RK commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1464525381)
suggestion, add

```cpp
if (group.m_weight <= max_input_weight) continue;
```
on top of the for loop instead of checking within each condition.

Currently, you add groups that are above weight to `applicable_groups`

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1907670816)
ACK fa4d9d5b39d5652de1d83564f19b1a0749863b16

There are many more tests possible, right? E.g. sending more than 4095 bytes of garbage.
I assume the goal here wasn't to cover everything?

✅ maflcko closed an issue: "Win64 CI failure in feature_versionbits_warning.py"
(https://github.com/bitcoin/bitcoin/issues/28115)