💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581128440)
I guess the "issue" is how people reading this in the future would differentiate an intended shadowing from an unintended one.` -Wshadow` will output a warning for this. Adding a special suppression tag just for this seems excessive to me.

But, in any case, it's not a big deal if you don't think it's worth it. I also don't think this is a problem anyway.

💬 fjahr commented on pull request "[Test] Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2079521684)
Code review ACK b259b0e8d360726b062c4b0453d1cf5a68e1933f

💬 iw4p commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2079523310)
Sorry, I'm a newbie. I read [this](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request) and only saw this tagging for PR title. Now I changed it to `refactor`.

💬 josibake commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079540754)
> Indexes match! If there's other implementations out there, we should do the same comparison.

Only other one I’m aware of is the cakewallet electrs fork, but they are using rust-silentpayments, which I veried is doing the outpoint sorting correctly (and passes @setavenger ’s new test case)

💬 kevkevinpal commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1581145534)
I added this line and it seemed to pass
```
assert_raises_process_error(1, 'timeout on transient error: Could not connect to the server 127.0.0.1:::', self.nodes[0].cli("-rpcconnect=127.0.0.1::").echo)
```
looks like `SplitHostPort` does not throw an exception if there are two colons and no port provided

I am unsure if that is needed logic in `SplitHostPort`

I'm seeing the same error when I do
`self.nodes[0].cli("-rpcconnect=127.0.0.256")`
probably because 255 is the max value

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581147730)
Much cleaner now

💬 sybenx commented on pull request "fix block subsidy at 3.25 BTC":
(https://github.com/bitcoin/bitcoin/pull/29778#issuecomment-2079554427)
3.125 is way too high. Once the risk becomes apparent, then a number would be obvious, if necessary

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581157473)
Why caching this in `m_recent_rejects_reconsiderable` instead of `m_recent_rejects` if we are not going to reconsider them?

💬 brunoerg commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2079566994)
Concept ACK

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1579748212)
nit: `tx_parent_3` is the same transaction `tx_parent_1` maybe just use `tx_parent_1` as parent of `tx_child_3`

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580896456)
I think this might be a better name
```suggestion
def test_package_rbf_with_conflicting_packages(self):
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580798337)
You moved `fill_mempool` to util in #29735

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1579508562)
Question: here you mean the package RBF rules in OP not BIP125 right?

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580946063)
nit:
```suggestion
def test_package_rbf_with_numerous_ancestors(self):
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580918022)
The parent transaction will be detected as `inmempool` as such the child just get added to the mempool if it passes normal transaction consensus and policy rules, the package does not have any conflict and hence package RBF rules are not checked.

This test also passes when I removed package RBF commits
```suggestion
def test_submitpackage_with_a_mempool_parent(self):
self.log.info("Test that submitpackage works when parent transactions was already submitted")
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580973594)
```suggestion
# Package 2 feerate is below the feerate of directly conflicted parent, even though
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1579749486)
`package1` and `package3` has the same parent

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1580970962)
nit:
```suggestion
def test_package_rbf_with_wrong_pkg_size(self):
```

💬 ismaelsadeeq commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1581088045)
nice this will be helpful for collecting data, previously we only know that a transaction is replaced now it indicates which tx replaced it!

E.g a transaction evicting two transaction package evicting another two
```terminal
2024-04-26T14:36:53.889903Z [httpworker.3] [validation.cpp:1282] [Finalize] [mempool] replacing mempool tx 1ae8118ceb71249b2af793f671a1293a4c999e89f5091f52c42a3cf1e08c6840 (wtxid=97fc8654dcbd025b4165c56179d124ee4adf2d65acf8a66f8f5464dab7d07089, fees=10000, vsize=104). N
...

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1581185454)
Ok, agreed