💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427410801)
Package are always topologically sorted, so am assuming this will always be best case no?

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427403296)
`IsConsistentPackage` in param is Package?
```suggestion
/** Construct a map from the txid of a transaction to the txids of its in-package ancestor set,
* including itself. Package must be IsConsistentPackage, otherwise this
* this returns an empty map. Package */
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427408824)
`if (curr_result_iter == ancestor_set_map.end()) return;` is redundant the map has already been populated with all package transactions, the statement will always be false.
So assert for that.
And return early when the transaction in-package ancestor set is not empty, that means its has been visited
```suggestion
auto curr_result_iter = ancestor_set_map.find(curr_txid);
// All transactions were populated to ancestor_set_map this should never happen
assert(curr_result_iter != an
...

💬 The-Sycorax commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1857098045)
The sheer amount of spam caused by inscriptions on the Bitcoin blockchain is actually quite alarming, it should be addressed comprehensively, and in a way that dose not harm the BRC-20 market or people's livelihoods.

Some individuals are not considering the nature of the vulnerability, it has nothing to do with getting rid of inscriptions. It has to do with the network congestion caused by these types transactions. It's not just a matter of higher fees or slower processing times; this situat
...

💬 ajtowns commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1427447113)
Reworded. Note there's lots of places in that file that talk about "bitcoind"...

💬 ajtowns commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1427448503)
Added a test; tagged as refactor for whatever that's worth

💬 ariard commented on pull request "Covenant tools softfork":
(https://github.com/bitcoin/bitcoin/pull/28550#issuecomment-1857107415)
To the best of my comprehension, most of the use-cases brought as a justification for the consensus changes introduced in this PR are relying on UTXO-sharing / time-locks as part of their design. I think most of them are affected by the following security & risks issues:
- [thundering herds](https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-September/004095.html)
- replacement cycling and pinning attacks
- [timewarp-based miners harvesting risks](https://delvingbitcoin.org/t/time
...

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1427479087)
> And, again, the user can set up a symlink from /tmp/test_common_Bitcoin\ Core/ to anywhere, if there's a desire not to use /tmp for some reason (but that should be rare).

Have you seen https://github.com/bitcoin/bitcoin/pull/26684#pullrequestreview-1566578747 and https://github.com/bitcoin/bitcoin/pull/28955#issuecomment-1829948567?
It seems that we need it.

> Well, if we want the datadir pathnames to be static (which is what I'm trying to accomplish with this PR), then we must delete t
...

📝 luke-jr opened a pull request: "refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition"
(https://github.com/bitcoin/bitcoin/pull/29086)
Instead of duplicating mempool options two places, just include the Options struct directly on the CTxMemPool

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1427525133)
good find! i've added a comment.

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1427525201)
changed it to send 0-9 number of decoy packets randomly.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1427525236)
good catch! changed it to `None`.

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1427525305)
oh makes sense to `s/ellswift_and_garbage_data/send_handshake_bytes` in `p2p.py`. didn't change the name in a test where we [send ellswift in parts](https://github.com/bitcoin/bitcoin/blob/ad0ae3d2128d04ff4f62a4bf612286d153dc314b/test/functional/p2p_v2_earlykeyresponse.py#L78).

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1427525347)
nice, i've kept it outside the loop since we don't need it anywhere else for now.

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

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1427525400)
true, replaced it with a check using `v2_receive_packet` return values which are anyways computed.

💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1857250302)
thanks for the reviews @mzumsande, @sipa, @theStack! i've updated the PR to address your comments. sorry for the delay since i was away.