💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2324984965)
> where all of our other peers don't support compact block

This, or a malicious peer somehow controls all three slots (harder since one slot is outbound), or we are in `-blocksonly` mode.

> In any case, this was non-obvious to me. Maybe add a comment in the spot where we fall back to GETDATA explaining that we don't wipe the partialBlock there on purpose?

Good idea, I will add a comment. I am not sure if the existing logic is on purpose, but the way it is written in master makes it impo
...

💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2324986541)
Good point, I will preserve the logic by disconnecting the peer.

💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2324985565)
> No access to `self.logger` here?

Yeah sorry, this is just a workaround for debugging. This test is just not working on windows and I was trying to debug it using CI to avoid needing to setup a windows build and vm and see if the issue happens there.

So far I've fixed one issue where unicode path CI uses was not properly handled by `bitcoin` wrapper on windows, but there is another issue where stdout/stderr process of the `bitcoind` do not seem to be captured by python when it is invoked
...

💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2319858536)
re: https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2313967295

Good catch, changed to bitcoin-qt. Also good to know text doesn't appear in gui --version output but I think that is ok as long as the test is only calling bitcoin node. I do think it's probably good for the Init::exeName method to return the right name for consistency even it's not called right now.

💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2319889144)
re: https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2313941771

> [5a28d0b](https://github.com/bitcoin/bitcoin/commit/5a28d0b027fdca07c55c853a550324c0aa1c450f): did you mean to add `-ipcconnect` and `-ipcfd` here?

Yeah they are not necessary but listed to be more future-proof. It makes sense for any IPC options to choose the IPC binary.

💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2319883129)
re: https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2313969321

> `bitcoin node` command?

Thanks, updated

💬 Sjors commented on issue "rpc: analyzepsbt should explicitly check for an invalid taproot_key_path_sig":
(https://github.com/bitcoin/bitcoin/issues/33320#issuecomment-3258218791)
FWIW this particular situation happened because a ledger signed one of the script paths and my WIP branch for HWI ended up putting that signature in the key path.

💬 Crypt-iQ commented on pull request "net: check for empty header before calling FillBlock":
(https://github.com/bitcoin/bitcoin/pull/33296#discussion_r2325023271)
It may be possible to call `reset()` on `partialBlock` to wipe it. I was assuming that wiping would mean calling `RemoveBlockRequest` (resets `m_downloading_since` which allows block download stalling) followed by `BlockRequested`. I will have to think more on this because I am not sure if it introduces a footgun.

👍 willcl-ark approved a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3189268483)
ACK faeb3320952906b6147b06170059e71d7d59f4bd

This looks correct to me.

For pushes we get the default behaviour, and for pull requests it evaluates to `github.ref` (e.g., `refs/pull/<pr-num>/merge`, ensuring the checkout uses the **dynamic** merge ref for the PR.

This avoids pinning to a specific commit SHA (on re-runs) and fetches the latest merge state.

💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2325049046)
Right. Fixed?

💬 TheCharlatan commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2325063247)
I think that should fix the problem, but the log line is still in the wrong place.

💬 fanquake commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2325069692)
This needs `python3-pip` in `PACKAGES`.

🤔 janb84 reviewed a pull request: "ci: reduce runner sizes on various jobs"
(https://github.com/bitcoin/bitcoin/pull/33319#pullrequestreview-3189341103)
ACK 5eeb2facbbbbf68a2c30ef9e6747e39c85d7b116

PR reduces select container sizes / cirrus runners. Not using unnecessary "large" runners is a good idea.

- code review ✅
- All CI builds run ✅

💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2325096198)
Right. More fixed?

💬 fanquake commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2325112783)
and `--break-system-packages`. Probably time to roll this job back into the CI, now that we've swtiched over.

💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2325115865)
Done. I was going to say, this doesn't seem to be invoked anywhere?

💬 fanquake commented on pull request "depends: disable builtin rules and suffixes.":
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3258373225)
> or could we use -R as well?

Pushed up a commit.

💬 stwenhao commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3258376429)
> and would avoid this issue without needing a (signet only) (consensus) code change.

Yes, but signet-only consensus code change can be beneficial. If your difficulty requires grinding 2^20 hashes, then I think nonce values should be masked, to make sure, that upper bits are set to zero. Then, if the difficulty will increase, the code can automatically raise that barrier, so that the nonce restrictions will be active only for difficulties requiring less than 2^32 hashes.

Because if you need to
...

💬 inkhaton commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-3258382540)
IT doesnt have to be some imaginary illegal number. Just the binary of a jpg thats child porn. With that every person running a node in most countries can be arrested.

🤔 janb84 reviewed a pull request: "ci: Checkout latest merged pulls"
(https://github.com/bitcoin/bitcoin/pull/33303#pullrequestreview-3189412062)
re ACK faeb3320952906b6147b06170059e71d7d59f4bd

Looks good !

changes sinds last ACK:
- using YAML_KEYS for templating with descriptive naming (moved from env)