🤔 itornaza reviewed a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2640357184)
rtACK fca0367d9a6f8a4296d41cc9b344c94b93ef136b

💬 maflcko commented on pull request "test: add coverage for abandoning unconfirmed transaction":
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2681443974)
> Covers the previously uncovered `TransactionCanBeAbandoned()` function increasing the coverage .

I don't understand this comment. `TransactionCanBeAbandoned` is only called by the GUI, but this new test isn't a gui test.

Also, https://corecheck.dev/bitcoin/bitcoin/pulls/31943 doesn't show new code coverage for `TransactionCanBeAbandoned` or anything else in wallet cpp

👍 hodlinator approved a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2640424818)
re-ACK fa99c3b544b631cfe34d52fb5e71636aedb1b423

Minor text-only changes + Fix for source of differing coverage-counters.

I'm fine with or without the `ResetCoverageCounters()`-change. Only meant to demonstrate that this tool was useful by testing it out in my last ACK.

💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969428698)
Static initialization should logically run first for linked libraries, but I just tested it and that was not the case. Code run during static initialization in tests was run before this lambda.

This means we could be accidentally hiding obscure cases/sources of non-determinism, but:
- If such early non-deterministic code has downstream effects on determinism of tests we will hopefully realize `ResetCoverageCounters` may be hiding something.
- If early code doesn't have downstream effects on
...

💬 Prabhat1308 commented on pull request "test: add coverage for abandoning unconfirmed transaction":
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2681502453)
> I don't understand this comment. `TransactionCanBeAbandoned` is only called by the GUI, but this new test isn't a gui test.

The confusion arose from my side . The RPC has the response `transaction not eligible for abandonment` and from the current coverage in the comment (which I thought to be the pre-pull coverage) , the marked line was already covered. So , I incidently thought the `TransactionCanBeAbandoned` to be the check for abandonment function refered to increase the coverage.

Re
...

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969500649)
Generally, tests shouldn't be using globals. When global state is used nonetheless, at least the (de-)initialization should happen in a well-defined order after `main`. Thus, I agree with your comment.

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969501913)
Closing thread for now, but I'll keep the follow-up in mind

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1969502508)
> I agree it can be done in a follow-up.

Closing thread for now, but I'll keep the follow-up in mind

👋 russeree's pull request is ready for review: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)

💬 rkrux commented on pull request "test: add coverage for abandoning unconfirmed transaction":
(https://github.com/bitcoin/bitcoin/pull/31943#issuecomment-2681599732)
> I don't understand this comment. TransactionCanBeAbandoned is only called by the GUI, but this new test isn't a gui test.

Correct, this doesn't cover `TransactionCanBeAbandoned`, which is called by GUI only. Instead, this covers the second condition in this if check here: https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/src/wallet/wallet.cpp#L1326C43-L1326C56

> Also, https://corecheck.dev/bitcoin/bitcoin/pulls/31943 doesn't show new code coverage for Trans
...

💬 laanwj commented on issue "Avoid plural forms in non-GUI translatable strings (lacks `%n` support)":
(https://github.com/bitcoin/bitcoin/issues/31890#issuecomment-2681613290)
Do we know how %n is implemented? Could this be added to tinyformat? Or does it rely on Qt specific state?

💬 rkrux commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-2681660181)
> But I found I needed to create a .lldbinit file with these lines:

@pinheadmz Has this fixed the mapping in your system permanently? I have not tried this yet and I observe that the source file detection fails because it seems to be generated in a new location everytime I build again like in the below errors.

```
Breakpoint at /Users/rkrux/projects/bitcoin/src/script/descriptor.cpp:1512 could not be resolved, but a valid location was found at /Users/rkrux/projects/bitcoin/build/src/script/des
...

📝 maflcko opened a pull request: "ci: [lint] Use Cirrus dockerfile cache"
(https://github.com/bitcoin/bitcoin/pull/31948)
The lint task is problematic, because:

* It doesn't check modifications to `ci/lint_imagefile`
* It calls a separate script that installs packages on every run (taking time)
* It uses `*_cache` instructions to cache some installed parts, but not all

Fix all issues by using `ci/lint_imagefile` (https://cirrus-ci.org/guide/docker-builder-vm/#dockerfile-as-a-ci-environment)

💬 l0rinc commented on pull request "log,optimization: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2681775327)
The code is reducing useless work therefore is also closer to how we'd likely write the code from scratch now.
It's basically a refactor that simplifies the happy path, with the side-effect of also being faster.

> as explained in the previous comments: https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2674257449 and https://github.com/bitcoin/bitcoin/pull/31923#pullrequestreview-2636342094

One was a question (I'm still measuing it, but don't expect to have any scenario where IBD
...

💬 itornaza commented on pull request "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0":
(https://github.com/bitcoin/bitcoin/pull/30994#issuecomment-2681780254)
Rebased and updated the description.

The errors are now suppressed by setting a flag for make in the `depends/hosts/darwin.mk` that make implementation of Xcode does not, see https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2582535849 and https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2602154368.

👋 itornaza's pull request is ready for review: "depends: fix for llvm-ranlib (etc): 'No such file or directory' macOS 15.0"
(https://github.com/bitcoin/bitcoin/pull/30994)

💬 rkrux commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2681967708)
Glad to see this PR merged!

📝 osrm opened a pull request: "doc: fix minor typo"
(https://github.com/bitcoin/bitcoin/pull/31949)
# Motivation
This PR fixes a typo: "occurences" is changed to "occurrences" to improve clarity and professionalism in the codebase. There are no tests associated with this typo change, as it is purely a spelling correction. Hope this helps.

✅ maflcko closed a pull request: "doc: fix minor typo"
(https://github.com/bitcoin/bitcoin/pull/31949)

💬 maflcko commented on pull request "doc: fix minor typo":
(https://github.com/bitcoin/bitcoin/pull/31949#issuecomment-2682029461)
Closing per CI failure. You'll have to submit upstream