💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581108542)
> This should be shadowing the previous `ancestors` variable (why the compiler isn't complaining about this?). The else path can also access the variable when it is declared inside the if statement.

I can rename the variable to something else if you have a suggestion. Shadowing is also present in the `qt/addresstablemodel.cpp` change. The shadowing is intended, and I don't think shadowing in cases like these is necessarily bad. I think shadowing is sometimes better than having multiple simila
...

💬 iw4p commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2079507177)
> If this is just a refactor, why the `fix:` on the commit message ([8c600f7](https://github.com/bitcoin/bitcoin/commit/8c600f7f78d5aaf3577f6257b648727d72e32a06))?

Usually the most prefixes are `fix` and `feat`, but sometimes some repos and projects use other keywords like chore, ref, etc.
[reference](https://www.conventionalcommits.org/en/v1.0.0-beta.2/).
By the way I modified the commit message prefix.

💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581122150)
Seems good enough

💬 fanquake commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2079513204)
You should read our developer notes for an overview of what is relevant to our project. ref doesn't mean anything here. You could probably use refactor:.

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