💬 hebasto commented on pull request "msvc: Optimize "Release" builds":
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848456277)
Friendly ping @sipsorcery :)

👍 hebasto approved a pull request: "util: Remove DirIsWritable, GetUniquePath"
(https://github.com/bitcoin/bitcoin/pull/28075#pullrequestreview-1773778627)
ACK fa3da629a1aebcb4500803d7417feed8e34285b0, I have reviewed the code and it looks OK.

💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1848591715)
@sipa Would you please review my PR? I really appreciate it.

💬 martinus commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1848609143)
I updated my PR by removing the changes that I did in initializer-lists. These were not necessary, evaluation order in these is well defined.

💬 sipsorcery commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848766633)
@hebasto what version of QT are you using for Windows builds?

It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.

💬 hebasto commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848769593)
> @hebasto what version of QT are you using for Windows builds?

Qt 5.15.11

> It's been a while since I did a QT build and I'm having lots of challenges with `5.15.11` on `master`.

I've made another Qt 5.15.11 build just today with no issues.

💬 sipsorcery commented on pull request "msvc: Define the same `QT_...` macros as in Autotools builds":
(https://github.com/bitcoin/bitcoin/pull/29044#issuecomment-1848781032)
tACK 1a5dae630df1eef9eac51557b2f1c5dba0924953.

💬 sipsorcery commented on pull request "msvc: Optimize "Release" builds":
(https://github.com/bitcoin/bitcoin/pull/29045#issuecomment-1848790814)
tACK 6e0f1d2abbb700d4fd4b956a7d1f9505b653653c.

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421613674)
Leftover comment?

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421615040)
Not sure if I'm missing something, but this only checks that the v3 inheritance rules are enforced among transactions in the package. What if someone sent a package of 2 v3 transactions, but the child transaction also spent a transaction that was already in the mempool? I think the `CheckV3Inheritance()` needs to be able to pull parents from both the mempool and from the package, perhaps?

💬 sdaftuar commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1421614259)
This is an aside -- in the cluster mempool branch, I think I'd like to change this to just look at direct parents rather than ancestors (I believe that in the normal course of transaction validation, we should no longer need to calculate ancestor sets when we're not worried about ancestor limits or updating cached ancestor state anymore). Can you think of any reason it would be problematic to replace ancestors with just an entry's direct parents here?

💬 sdaftuar commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1421668681)
I wouldn't say it's safe to ignore, but the idea is that we often want to be able to batch deletions, and then clean things up in one pass. So the call sites should all be dealing with this issue and ensuring that we always clean up at some point.

(This is definitely an area where I expect that we'll be re-engineering all this logic and trying to come up with a better abstraction layer so that this is more robust and easier to think about!)

💬 kevkevinpal commented on pull request "Add multiplication operator to CFeeRate":
(https://github.com/bitcoin/bitcoin/pull/29037#issuecomment-1848832142)
ACK [1553c80](https://github.com/bitcoin/bitcoin/pull/29037/commits/1553c8078698df1058b62e8fdadaf74160977b30)

💬 luke-jr commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1848850110)
The vulnerability this fixes has been assigned CVE-2023-50428

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1421680771)
Done

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#discussion_r1421680795)
Done

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1421685637)
Done

💬 conduition commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1848866953)
Concept NACK

I personally have no use for on-chain ordinals or inscriptions. I think everyone would be better off if these were moved to some off-chain or side-chain protocol. However I can't rightly label the inscriptions as 'spam', nor would I dub their exclusion from the mempool 'censorship'. The distinction between 'censorship' and 'spam filtering' is entirely subjective.

If i publish a chain of 1000 transactions pointlessly bouncing coins around my own wallet addresses, is that spam?
...

⚠️ ArmchairCryptologist opened an issue: "Nit: Inconsistency in the docs regarding blocks-relay-only connections"
(https://github.com/bitcoin/bitcoin/issues/29046)
I was looking into why my node was frequently opening new block-relay-only connections even though it already had two such nodes connected, and while this appears to be intended behavior to counter eclipse attacks, while searching the codebase for this I encountered a minor contradiction in the docs.

Specifically, [doc/reduce-memory.md](https://github.com/bitcoin/bitcoin/blob/3e691258d8789a4a89cce42e7e71b130491594d7/doc/reduce-memory.md?plain=1#L29) says:

```
- `-maxconnections=<n>` - the
...

💬 TheCharlatan commented on pull request "mempool: Don't sort in entryAll":
(https://github.com/bitcoin/bitcoin/pull/29019#discussion_r1421721623)
Thanks, seems easy enough to re-ACK, so will push this.