💬 maflcko commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r2590403179)
It is an internal bug, so it is dead code and doesn't really matter much. I think internal bugs should be using `Assume(false)`. Logs could go with `LogError("%s", STR_INTERNAL_BUG("..."))`, but I wanted to keep the changes here minimal. Happy to go with `LogError("%s", STR_INTERNAL_BUG(` if you feel strongly.

🤔 darosior reviewed a pull request: "test: Add DERSIG unit tests to script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3541895187)
Makes sense to test `DERSIG` separately from `STRICTENC` since it's the only one enabled by consensus. Concept/Approach ACK.

Maintainers could we get the CI ran here?

💬 ryanofsky commented on pull request "test: interface_ipc.py minor fixes and cleanup":
(https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3614166836)
<!-- begin push-2 -->
Updated d179f713792432c82befd39a288ab16fec13bab1 -> 6ef092c0e4343fc421c323ff09d3c791fb4bc33a ([`pr/pyipc.1`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.1) -> [`pr/pyipc.2`](https://github.com/ryanofsky/bitcoin/commits/pr/pyipc.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/pyipc.1..pr/pyipc.2))<!-- end --> splitting into two commits

re: https://github.com/bitcoin/bitcoin/pull/34003#issuecomment-3611494379

> > Destroy calls were being made at
...

🤔 stickies-v reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3541953136)
Concept ACK. Functions with return type `std::optional<E>` returning a truthy value on failure is unintuitive, this is much better.

👍 darosior approved a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541954260)
utACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

🤔 mzumsande reviewed a pull request: "net: fix use-after-free with v2->v1 reconnection logic"
(https://github.com/bitcoin/bitcoin/pull/33956#pullrequestreview-3541981127)
ACK 167df7a98c8514da6979d45e58fcdcbd0733b8fe

didn't test it, but but the explanation of the bug make sense and since all the network threads have been stopped at this point anyway I can't see a downside to clearing `m_reconnections` here.

👍 maflcko approved a pull request: "test: Remove `system_tests/run_command` runtime dependencies"
(https://github.com/bitcoin/bitcoin/pull/33929#pullrequestreview-3542024077)
lgtm

💬 maflcko commented on pull request "test: Remove `system_tests/run_command` runtime dependencies":
(https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065)
nit: This will fail with spaces in the dir:


```
$ ./a\ space/test_bitcoin -t system_tests
Running 1 test case...
unknown location(0): fatal error: in "system_tests/run_command": subprocess::CalledProcessError: execve failed: No such file or directory (2)
test/system_tests.cpp(29): last checkpoint

*** 1 failure is detected in the test module "Bitcoin Core Test Suite"

👍 sedited approved a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3542392743)
ACK 9d5021a05bd33c73276909eec961777867ddb412

👍 andrewtoth approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3542841874)
ACK 2d398050ee31db05e9222149b5e60572ac31d803

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591174137)
nit
```suggestion
const auto pushed_tx_opt{m_tx_for_private_broadcast.GetTxForNode(pfrom.GetId())};
```

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591173094)
nit
```suggestion
const auto opt_tx{m_tx_for_private_broadcast.PickTxForSend(node.GetId())};
```

💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2591176187)
nit
```suggestion
if (const auto num_broadcasted{m_tx_for_private_broadcast.Remove(ptx)}) {
```

⚠️ fugatecorey22 opened an issue: "Admin Dashboard"
(https://github.com/bitcoin/bitcoin/issues/34009)
npm create vite@latest myapp --template react
cd myapp
npm install
npm run dev

📝 rkrux opened a pull request: "wallet: check for `agg_pub` validity in MuSig2 signing"
(https://github.com/bitcoin/bitcoin/pull/34010)
Otherwise conversion of an invalid `agg_pub` to `XOnlyPubKey` in a later step few lines below crashes the flow.

Should fix #33999.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improv
...

👍 sedited approved a pull request: "test: interface_ipc.py minor fixes and cleanup"
(https://github.com/bitcoin/bitcoin/pull/34003#pullrequestreview-3543353009)
ACK 6ef092c0e4343fc421c323ff09d3c791fb4bc33a

Thanks for the cleanup! Have another suggestion for saving a few lines and naming clarity:

<details>
<summary>Diff</summary>

```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 804f7f8f64..f8f3a76ec7 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -176,2 +176 @@ class IPCInterfaceTest(BitcoinTestFramework):
- template3 = await template2.wa
...

💬 sedited commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3615621559)
> What is wrong with the most obvious approach to error handling?

I am guessing you are asking why not just use exceptions? If so, I think they could be an alternative for these situations too. I'd be curious how a similar function contract could be enforced with an exception.

👍 sedited approved a pull request: "depends: Propagate native C compiler to `sqlite` package"
(https://github.com/bitcoin/bitcoin/pull/33995#pullrequestreview-3543468882)
ACK 710031ebef838d2f0a1effa19170bef7b130bbeb

💬 plebhash commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3615719206)
during some IRL discussions with @ismaelsadeeq and @Shourya742 we brainstormed some potential alternative approach to this

instead of trying to establish boundaries on how much memory would be consumed with templates, we could simply start from the assumption that any economically rational implementation always aims to be hashing on whatever is the latest (and therefore most profitable) template

while the most conservative approach would try to keep as many templates as possible in memory (bec
...

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#issuecomment-3615779397)
> just use exceptions?

I don't think exceptions are a suitable alternative for low-level code in this repo, where `std::expected` is appropriate. In places where exceptions are used today, it is tedious to review the code and its behavior. For example, the same exception from an IO error in a database will sometimes terminate the program unclean, and sometimes cleanly shut down the program (https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3116540946), depending on which thread or wi
...