💬 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_r2558059839)
I actually slightly prefer to keep it as is, because if I do this earlier I also have to change the tests because the hash result changes too with the span usage. I think it makes more sense to have this in the later commit where there is a much bigger overall benefit of introducing span evident that is actually what justifies the hash result being changed.

💬 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_r2558060398)
Renamed to `CheckStandardAsmap` and using it in a two more places, but keeping it out of asmap_direct fuzzer because we are testing the bits there explicitly. Moved it below below the sanity check in .h which is the same location as .cpp

💬 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_r2558060593)
Done

💬 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_r2558060750)
Done

💬 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