💬 laanwj commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#discussion_r1581051463)
It's important to be careful about behavior changes but i do think this is an improvement, using `requests.head` is better practice than calling curl then looking for a string in the output.

💬 brunoerg commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#issuecomment-2079416479)
Rebased to re-run CI.

💬 brunoerg commented on pull request "refactor: convert string formatting to F-strings":
(https://github.com/bitcoin/bitcoin/pull/29969#issuecomment-2079418264)
If this is just a refactor, why the `fix:` on commit message?

💬 laanwj commented on pull request "refactor: switch from curl to requests for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2079426084)
This is a script used for the tests, so adding `test` tag.

💬 maflcko commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1581039091)
nit: Is this line needed?

💬 fjahr commented on pull request "[Test] Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2079432927)
```
test/functional/feature_assumeutxo.py:401:1: W293 blank line contains whitespace
^---- failure generated from lint-python.py
```

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

💬 theuni commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2079433585)
Neat!

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