💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581069584)
useful log message for stats :+1:

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581019326)
redundant comment

💬 iw4p commented on pull request "test: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2079456810)
The title's tag also changed from `Refactor` to `Test`

💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581108051)
Nope, it's nice and quiet.

💬 Sjors commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1581108497)
Yes, it's fine.

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