💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2558061851)
Thanks, I have applied this latest patch, I think that also resolves your earlier comment that was in a different comment thread.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3573190858)
Addressed the latest review comments from @hodlinator , thanks for the detailed patch suggestions and rationale, co-authorship added!

💬 plebhash commented on pull request "[30.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3573215231)
please add https://github.com/bitcoin/bitcoin/pull/33676, it would be highly appreciated as a way to expedite https://github.com/stratum-mining/sv2-apps/issues/81

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3573340203)
Addressed comments and rebased after #33770 including the latest changes in #33878. This PR now necessarily re-adds the possibility to set a bool option for `-asmap` to make it possible to use asmap without an external file. I have done this a bit differently now and I think it's better.

Re @hodlinator https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2549632572: I don't think I understand your comment. I am pretty sure I have addressed your previous comment to use `OFF` but the file
...

💬 fjahr commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2558166994)
Done

💬 frankomosh commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#issuecomment-3573874026)
ACK c34bc01

💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3574043761)
@pablomartin4btc I've just tried your code from this branch and it works fine on top of the current master, take a look:

https://github.com/user-attachments/assets/a6dd0b43-15c0-42de-b379-598a581ab44a

💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3574067005)
@pablomartin4btc like I said, this variation when applied to your branch also makes it work:

```diff
diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
index 48e41bc352..72c9a65cd3 100644
--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -36,6 +36,7 @@
#include <QScrollBar>
#include <QSettings>
#include <QString>
+#include <QWindow>
#include <QTableView>
#include <QTimer>
#include <QUrl>
@@ -671,7 +672,9 @@ bool TransactionView::detailsA
...

💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3574153166)
@pablomartin4btc this also works:

```diff
diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
index 48e41bc352..e8b42d4983 100644
--- a/src/qt/transactionview.cpp
+++ b/src/qt/transactionview.cpp
@@ -671,7 +671,7 @@ bool TransactionView::detailsAlreadyShown(const QModelIndex &idx)
{
for (TransactionDescDialog* dlg : m_opened_dialogs) {
if (dlg->getTransactionId() == idx.data(TransactionTableModel::TxHashRole).toString()) {
- GUIUtil::bringTo
...

💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3574162074)
`dlg->activateWindow()` is essentially the same as calling `GUIUtil::bringToFront(dlg)`.

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2558941954)
I was pointing to this sentence in the PR description at the top, where it's still using "NO" rather than "OFF", as was changed in the code:
> By default the data at that location is embedded into the binary but there is also a build option to prevent this (`-DWITH_EMBEDDED_ASMAP=NO`).

(You can respond by jumping here: https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514815293).

✅ laanwj closed an issue: "guix build fails on RISC-V due to python-py-cpuinfo test failure"
(https://github.com/bitcoin/bitcoin/issues/33873)

💬 laanwj commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3574292725)
Clearly a hardware issue, closing.

🤔 rkrux requested changes to a pull request: "wallet: Expand MuSig test coverage and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33636#pullrequestreview-3503877587)
Thanks for accepting the refactoring suggestions!

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2559047569)
In b7215893e566dec8a085c08e722aa61ec5a2f5a0 "test: Add musig failure scenarios"

The `self.test_failure_case_3("mismatched key order", "tr(musig($0/<0;1>/*,$1/<1;2>/*))")` case seems incorrect to me because the participant pubkeys are sorted before generating the pubkey (and the address) that should result in both the wallets having same address.
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/script/descriptor.cpp#L658

This test case is passing because
...

👍 TheCharlatan approved a pull request: "Add a "tx output spender" index"
(https://github.com/bitcoin/bitcoin/pull/24539#pullrequestreview-3503919394)
re-ACK 38f2f53963e833d25bc71df0713fb28cff17cc43

🤔 l0rinc reviewed a pull request: "contrib: turn off compression of macOS SDK to fix determinism (across distros)"
(https://github.com/bitcoin/bitcoin/pull/32009#pullrequestreview-3503804718)
I don't yet have experience with this part of the code, left a few nits.
I arrived here while investigating an upgrade to Xcode 16.3 in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2550717279 and
noticed a couple of potential issues around the `argparse` usage and the `-o`
handling.

I have tested the following changes locally:

```patch
diff --git a/contrib/macdeploy/gen-sdk.py b/contrib/macdeploy/gen-sdk.py
index 426d82e46c..3bf9154887 100755
--- a/contrib/macdeploy/gen-sdk.py
++
...

💬 l0rinc commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559089068)
are these file extensions? I wasn't familiar with them, but e.g. https://medium.com/@mail2ashislaha/swift-objective-c-interoperability-static-libraries-modulemap-etc-39caa77ce1fc indicates they're extensions, in which case:

```suggestion
if tarinfo.name and tarinfo.name.endswith((".swiftmodule", ".modulemap")):
```

💬 l0rinc commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2559130969)
are we reassigning `out_sdkt_path` here? Based on the condition checking `args.out_sdkt`, shouldn't this rather be:
```suggestion
if args.out_sdkt:
out_sdkt_path = pathlib.Path(args.out_sdkt[0])
```

or maybe if we remove `nargs=1` from above we could do:
```suggestion
if args.out_sdkt:
out_sdkt_path = pathlib.Path(args.out_sdkt)
```

Was this working before? I don't really have experience with this, so I can't tell...

💬 l0rinc commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#discussion_r2558987253)
should we adjust the help description as well?
```suggestion
parser.add_argument("-o", metavar='OUTSDKTAR', nargs=1, dest='out_sdkt', required=False)
```

> usage: gen-sdk.py [-h] [-o OUTSDKTAR] XCODEAPP