💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128297)
Done

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128370)
Done

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128432)
I've added a comment.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390128566)
Fixed

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390129010)
I've added this comment explaining why 10 blocks need to be mined:
```
# mine 10 blocks so that when the blk is invalidated, the transactions are not
# returned to the mempool
```

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1390129173)
I've replaced `tx1` with `tx1_txid` here and for all other instances of this.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1806687648)
Rebased and addressed most of the review comments (thanks @achow101 and @murchandamus). I will address the remaining comments soon.

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1390149597)
nice! included `MAGIC_BYTES` in messages.py

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

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1390149630)
but there's also an else condition below where if the `received_garbage` doesn't match the garbage terminator, we keep appending 1 byte to `received_garbage` until there's a match with garbage terminator/we've exceed max garbage limit. so even though initially `received_garbage` length is 16 bytes, it can take values from 17 bytes, ..., 17 + 4095 bytes.

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

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

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1806715075)
@theStack, @mzumsande thank you for the reviews! I've rebased and addressed your comments.

> It seems like the newly introduced functional test p2p_v2_encrypted.py could be merged with the already existing p2p_v2_transport.py (here or in a follow-up), or is it fundamentally different?

yes! can be done in a follow up. only difference is `p2p_v2_encrypted.py` tests `TestNode` <--> `P2PInterface` behaviour and `p2p_v2_transport.py` tests `TestNode` <--> `TestNode` behaviour.

@maflcko, sti
...

💬 hebasto commented on pull request "depends: remove `PYTHONPATH` from config.site":
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806743379)
My Guix builds:
```
x86_64
92c922498b9d7e68742355653cc84317d0f0947e8fdd012488c9875e8b54e03c guix-build-95aab1f02784/output/aarch64-linux-gnu/SHA256SUMS.part
634a6a7a82efcb86eb7e875aa4534bd5646e80ee21e951d9624ca7ff78fc82f5 guix-build-95aab1f02784/output/aarch64-linux-gnu/bitcoin-95aab1f02784-aarch64-linux-gnu-debug.tar.gz
2dd4305e325723cac4b0b8e1803d48f3d7eb6fa2fd49c0a980ff907bb3e5da2b guix-build-95aab1f02784/output/aarch64-linux-gnu/bitcoin-95aab1f02784-aarch64-linux-gnu.tar.gz
b530c235
...

💬 TheCharlatan commented on pull request "[refactor] Remove BlockAssembler m_mempool member":
(https://github.com/bitcoin/bitcoin/pull/28843#discussion_r1390177609)
Looking at the review discussion in #25223 introducing a check in the function and using the arrow operator feels like a step back in code quality. I also feel like having raw pointers as class members is something we should try to avoid, since its ownership semantics are ambiguous. Relying on the member variable precludes this later [comment](https://github.com/bitcoin/bitcoin/pull/25223#discussion_r898233059) on the one I linked in the PR description by making a refactor into fully static func
...

👍 hebasto approved a pull request: "depends: remove `PYTHONPATH` from config.site"
(https://github.com/bitcoin/bitcoin/pull/28845#pullrequestreview-1726010149)
ACK 95aab1f02784e5d92572ccfcd4ce27107e4b2888, setting `PYTHONPATH` is indeed not needed since https://github.com/bitcoin/bitcoin/pull/28432.

💬 hebasto commented on pull request "depends: remove `PYTHONPATH` from config.site":
(https://github.com/bitcoin/bitcoin/pull/28845#issuecomment-1806750242)
Could the https://github.com/bitcoin/bitcoin/commit/de619a37fd18a17225c8a10b828fc61958abe4cf be reverted?