💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499341)
Unified.

> Right now, FillSignatureData is only called on new empty SignatureData objects, but not sure if that is an assumption in the code.

That is not an assumption.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499483)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383499646)
Done

💬 Fiach-Dubh commented on something "":
(https://github.com/bitcoin/bitcoin/commit/766561b8297b534dac1ed7ccfc57e03fa5a41043#commitcomment-166657363)
we can't give luke-jr the nuclear launch codes!

🤔 hodlinator reviewed a pull request: "qa: Improvements to debug_assert_log + busy_wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3273948902)
Latest push fixes bug from previous push (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379926157), simplifies code (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379920746) and adds commit to fix docstring (https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379958513).

Compared run-times for all functional tests and wasn't able to find one which clearly benefited from this optimization. So I'm open to just replacing `while remaining_expected and remaining_ex
...

💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383490859)
Regarding nit: I sympathize with the way `busy_wait_for_debug_log()` on master puts the parameter to `_raise_assertion_error()` on it's own line, so I would be tempted to do that for all of them - which makes me realize: if only I broke the message in two lines, I could raise up the first half and still keep close to 80 char width - and we're back with the current approach. Left unchanged in latest push.

💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383490580)
Thanks for noticing this issue!

Latest push inserts
```python
remaining_expected = [e for e in remaining_expected if e not in log]
```
before the error reporting for it to be correct, meaning we only need to do the full search on failure.

💬 hodlinator commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2383497595)
Thanks! Added commit removing this from docstring with reference in commit message to ref commit where it was made incorrect.

👍 hodlinator approved a pull request: "Modernize use of UTF-8 in Windows code"
(https://github.com/bitcoin/bitcoin/pull/32380#pullrequestreview-3274017245)
re-ACK 63f3ea2b041db56487bfb92813e32c0bfef6faa1

Confirmed to resolve conflict with #33229.

💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3340619538)
If this is backported, release notes https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft#ipc-mining-interface could be be simplified to not mention `-m` at all, which would be nice

💬 polespinasa commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#discussion_r2383565725)
While some miners may want to not include large OP_RETURNS, I think we should discourage it to miners too.

Miners filtering transactions will probably take more time to start working on the new tip, which is bad for them. This should be a reason to discourage the use for miners.

> And the miner use case doesn't have a natural end of life argument...

See that my proposal is to only discourage the use (always) not to plan for removal.

💬 achow101 commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3340629533)
Can you do the version bump stuff here?

🤔 l0rinc reviewed a pull request: "node: Add --debug_runs/-waitfordebugger + --debug_cmd"
(https://github.com/bitcoin/bitcoin/pull/31723#pullrequestreview-3273781326)
Concept ACK, I'm always struggling with debugging these tests with multiple nodes, I usually end up just printing their internal states.

Thanks for working on this. I went through the code, tried to find simpler solutions to minimize and modernize the code we need to add to production code. I didn't get to trying this out yet, so I'm not sure my suggestions make sense, I just checked if it compiles on a Mac or Linux. Will try to test it properly next week.

Since we're editing production code f
...

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383388859)
nit: comment is redundant

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383387627)
the `0-based` part may need some explanation

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383397007)
[This](https://learn.microsoft.com/en-us/windows/win32/api/debugapi/nf-debugapi-isdebuggerpresent) is funny, I wonder why other platforms don't have this.

Seems C++26 will have this standardized: https://en.cppreference.com/w/cpp/utility/is_debugger_present.html, maybe we can add it as a comment (and adjust the naming to match that, if needed)

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383391233)
nice - it might be overkill, but how difficult do you think it would be to add a test for whether we can start a test with debugging? Would help with checking it on other platforms as well

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383394089)
learning from https://github.com/bitcoin/bitcoin/pull/33435 we may want to add support for BSD as well (not sure anyone will use it, but at least we should be aware that it's an option)

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383440187)
nit: any reason for not sorting these?

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2383468341)
I wonder if there's a better way than sleep. The [internets](https://www.oreilly.com/library/view/secure-programming-cookbook/0596003943/ch12s13.html) indicates that `raise(SIGSTOP)` + `SIGTRAP` would also pause and revive the process safely - I don't have Linux locally, but maybe we could give it a try