💬 maflcko commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1427346763)
Yes. (Not here, obviously, but in a follow-up possibly)

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1427351949)
this looks like a good test to add to tease out the differences in code paths

💬 maflcko commented on pull request "refactor: Remove c_str from util/check":
(https://github.com/bitcoin/bitcoin/pull/26960#issuecomment-1856761834)
> If so, nit: worth adding a little `// TODO` comment to document that with C++20 we can use `consteval` and `string_view`? Personally, I find those quite helpful.

I tried this and it seems to compile, but I don't think it is worth it:

```diff
diff --git a/src/util/check.h b/src/util/check.h
index a02a1de8dc..f15117daef 100644
--- a/src/util/check.h
+++ b/src/util/check.h
@@ -21,9 +21,11 @@ public:
NonFatalCheckError(std::string_view msg, std::string_view file, int line, std::st
...

💬 amitiuttarwar commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1856825484)
ACK ad48667ec8e8d563550df768d0b45abf800662d9

📝 fjahr opened a pull request: "refactor: C++20: Use lambda implicit capture and std::rotl"
(https://github.com/bitcoin/bitcoin/pull/29085)
While exploring some C++20 changes and checking against our code I found these two potential improvements:

1. We can replace our custom implementation of `rotl32` in crypto/chacha20 with `std::rotl` from the [new `bit` header](https://en.cppreference.com/w/cpp/header/bit).
2. Implicit capture of `this` in lambdas [is deprecated](https://en.cppreference.com/w/cpp/language/lambda#Lambda_capture). The capture will need to be made explicit in the future, so change this now.

💬 kashifs commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1856889095)
Okay, great! I'm slowly working my way through #27877

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