💬 maflcko commented on pull request "refactor: Avoid unused-variable warning in init.cpp":
(https://github.com/bitcoin/bitcoin/pull/29968#discussion_r1581066427)
> Or maybe, even check it and raise an error. Eg.

Sure, happy to close this pull if someone creates an alternative, but I'll probably leave this pull as-is

💬 furszy commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581073867)
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.

💬 alfonsoromanz commented on pull request "[Test] Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2079445888)
Thanks @fjahr. I just fixed the linting issue.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2079450470)
I currently don’t sort or actually the ordering gets messed up due to parallel processing. I will probably include the possibility to sort the tweaks in the array.

The cut-through tweak index stores the txid for every tweak, so sorting them might not be that necessary. The cut through index also includes metadata like a dust indicator.

👍 instagibbs approved a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2025023787)
reACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

non-blocking comments

💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581036209)
now that we have `PackageToValidate`, just directly pass here?

💬 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