💬 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?

💬 glozow commented on pull request "bugfix, Change up submitpackage results to return results for all transactions":
(https://github.com/bitcoin/bitcoin/pull/28848#discussion_r1390186647)
Maybe add an "allowed" field for the full package, for convenience? Otherwise the user has to iterate through all the results to find out whether everything got submitted (which is likely the most common result in practice?)

💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1390234612)
Oh right, I missed the else-branch and assumed `received_garbage` wouldn't change in the loop (which wouldn't make much sense), nevermind.