💬 0xB10C commented on issue "signet: disk-space-DoS due to low mining difficulty":
(https://github.com/bitcoin/bitcoin/issues/33266#issuecomment-3258202695)
My impression is that just increasing the difficulty on a public Signet should be enough and would avoid this issue without needing a (signet only) (consensus) code change. It doesn't even need to be difficulty 1 (i.e. 2^32 work) - similar to the default signet only requiring 5% of work at the risk of 19 alternative blocks per tip. The attack is easily detectable and if exploited over a long period of time, the signet admin can further increase the difficulty at any time. Alternatively, publishi
...

⚠️ Sjors opened an issue: "rpc: analyzepsbt should explicitly check for an invalid taproot_key_path_sig"
(https://github.com/bitcoin/bitcoin/issues/33320)
While testing #29675 my final combined PSBT ended up having an invalid `taproot_key_path_sig`, which I found out by logging the `sigdata.complete` value at the end of `ProduceSignature`.

We should of course never produce such an invalid value and I'll continue digging for the root cause. But external applications could do this too.

Currently `analyzepsbt` will give a useless `"next": "updater"` status when this happens.

We should instead display an error.

Not sure what the best approach woul
...

💬 TheCharlatan commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2324982685)
Sorry for the churn, but I think I made a mistake here:
```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index a87ada4ffa..8410fa24be 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -149,0 +150 @@ class IPCInterfaceTest(BitcoinTestFramework):
+ self.log.debug("Wait for another, but time out")
@@ -155,2 +156 @@ class IPCInterfaceTest(BitcoinTestFramework):
- self.log.debug("Wait for anot
...

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