💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1484800862)
> If we get the same txid but different wtxid, I'm not sure that the result is necessarily one that makes sense. Most things in the wallet are keyed and listed by txid, so with this change, we'd end up with a transaction that's listed as conflicted by it's own txid. Also, we won't ever update the wallet to store a transaction with a different wtxid.

Sorry I didn't see this earlier. After looking at this again I don't think that same-txid-different-witness will be a problem here. Please see: h
...

💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1936624217)
ACK [13161ec](https://github.com/bitcoin/bitcoin/pull/27877/commits/13161ecf032b7a850686e5942c12222c8f3d0d52)

🚀 achow101 merged a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877)

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1936659009)
> looks like if I revert TX_MAX_STANDARD_VERSION to 3, all unit/functional tests pass?

I suppose my change at https://github.com/bitcoin/bitcoin/pull/28948/files#diff-cc0c6a9039a1c9fe38b8a21fe28391fffbac9b8531dfda0f658919a9f74b46baR793 is partially to blame. I made it `TX_MAX_STANDARD_VERSION + 1` instead of 3, thinking it might be easier to flip in the future.

I'm working on a followup to address the comments, and can incorporate if I need to retouch.

🤔 glozow reviewed a pull request: "p2p: Don't process mutated blocks"
(https://github.com/bitcoin/bitcoin/pull/29412#pullrequestreview-1873261879)
concept ACK, I agree with the approach of catching and dropping these earlier rather than later

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484847574)
Yes, seems this is because `SingleV3Checks` already covers it. I can add a comment and put the condition under an `Assume`?

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1936684548)
> I'm seeing an issue during testing. The debug log incorrectly reports 0 MB. Are others able to reproduce my result?
>
> ```
> user1@comp1:~/Documents/GitHub/btc29402$ src/bitcoin-cli --rpcwait getmempoolinfo | jq -c '{ transactions: (.size), memory_usage_in_mb: (.usage/1000000) }' | jq; src/bitcoin-cli --rpcwait stop; sleep 10; grep -a Writing ~/.bitcoin/debug.log | tail -2
> {
> "transactions": 2529,
> "memory_usage_in_mb": 6.061648
> }
> Bitcoin Core stopping
> 2024-02-09T14:11
...

💬 epiccurious commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1936688373)
> it might make more sense to not have a MB amount on vinfo and print it out for the file size at the end of the creating the mempool file so users are not confused when they see xx amounts in MB and their file actually a different size

Approach ACK.

💬 kevkevinpal commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#issuecomment-1936745081)
rebased and also updated code to get the size of the file after saving

logs now look like such and I tested to see that the `mempool.dat` file was the correct size by going to my data dir and doing `ls -alh` and they matched in sizes
```
2024-02-09T23:41:52Z DumpAnchors: Flush 2 outbound block-relay-only peer addresses to anchors.dat completed (0.02s)
2024-02-09T23:41:52Z scheduler thread exit
2024-02-09T23:41:52Z Writing 29 mempool transactions to disk...
2024-02-09T23:41:52Z Writing 0
...

💬 achow101 commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1936857327)
ACK 29029df5c700e6940c712028303761d91ae15847

🚀 achow101 merged a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948)

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484980137)
Mh, it could also be turned on after max_weight was exceeded the first time. :thinking: Might follow-up on this.

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484992098)
nit: This is obvious I think, not really adding much, comment should be below explaining why we are assuming descendant count is 1 below
```suggestion
```

🤔 ismaelsadeeq reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1873486217)
Post merge ACK

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484992336)
nit: same here I think
```suggestion
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484994642)
The magic number can be declared with a descriptive variable name,
MAX_PUBKEYS_PER_MULTISIG = 20
bytespersigop = 5000

We can reuse them in the same function
```suggestion
expected_package_error = f"package-mempool-limits, package size {2*MAX_PUBKEYS_PER_MULTISIG*bytespersigop} exceeds ancestor size limit [limit: 101000]"
```

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484995474)
@instagibbs To test my understanding of the discussion here I created a test for this case https://github.com/ismaelsadeeq/bitcoin/commit/87fc7f0a8d8ad1fc2af60ee6cc678df5e6fb01dd

I agree we should not force siblings tx to conflict with reorged siblings, since we dont enforce v3 rules after reorg.

Maybe this should be documented in the comment.

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1936910213)
Code review ACK 83f34b4783565336341948affa42e3ec71242c4b

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484996733)
Note a good idea, because we have to restart whenever we want to update the threshold for an RPC call, can be resolved.

💬 ismaelsadeeq commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1484997792)
I understand why we should not check the package fee rate here because, in some arrangements, e.g., a parent transaction P will be cfp'ed by some child E, and E can have another child F, which is on its own.
Checking the package fee rate in this case is incorrect; we should instead evaluate using chunks fee rate later, as you said 👍🏾