💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2053833598)
Thanks for the review @rkrux !

> Suggested adding a log message in the last portion of the test.

Nice suggestion, I did this just now when I rebased on latest `master`

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1564364788)
Yeah there is def some duplicated code between the two tests. But I want to focus on making this simple and easy to understand. It's trying to demonstrate/document an example wallet and could be used as a reference. Kind of like a quick start / tutorial

If I were to combine these tests it would become harder for someone to quickly and easily follow, and would lose the purpose of being a quick example. I think it's quite common to have code duplication in these types of tests/examples for that
...

🤔 tdb3 reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-1999403287)
Concept ACK.
Great feature to help prevent conflicting datadir conflicts when switching between signets.
Inline comments added.

Received failures for functional tests feature_signet.py and tool_signet_miner.py.
```
dev@bdev01:~/bitcoin$ test/functional/test_runner.py -u --cachedir=/mnt/tmp/cache --tmpdir=/mnt/tmp test/functional/feature_signet.py test/functional/tool_signet_miner.py
Temporary test directory at /mnt/tmp/test_runner_₿_🏃_20240413_214701
Remaining jobs: [feature_signet.py
...

💬 tdb3 commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r1564386036)
The approach to use the 8-character message start (i.e. first 4 bytes of the SHA256 hash of the challenge) seems better to me as well. More concise directory lengths with a reasonably low chance of collision between signets.

nit: Recommend avoiding using `-` (dash) and instead using something like `_` (underscore) when building signet paths to prevent unexpected issues when including custom signet directory paths in command line arguments.

💬 kristapsk commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-2053925953)
Concept ACK

💬 furszy commented on issue "test: intermittent issue in interface_rest.py - AssertionError: self.wait_until(lambda: self.nodes[0].getindexinfo() == expected_filter)":
(https://github.com/bitcoin/bitcoin/issues/29863#issuecomment-2053935685)
Same issue as #29831, logs:
```
node0 2024-04-12T09:45:29.827143Z [scheduler] [validationinterface.cpp:212] [operator()] [validation] BlockConnected: block hash=54586dbb0de750d4bcd83326f04f6619d8e00dd5815981030ace95125f1eef87 block height=208
...
node0 2024-04-12T09:45:29.827159Z [scheduler] [index/base.cpp:293] [BlockConnected] BlockConnected: WARNING: Block 54586dbb0de750d4bcd83326f04f6619d8e00dd5815981030ace95125f1eef87 does not connect to an ancestor of known best chain (tip=39455d83324
...

👍 hebasto approved a pull request: "minisketch: update subtree to 3472e2f5ec75ace39ce9243af6b3fee233a67492"
(https://github.com/bitcoin/bitcoin/pull/29823#pullrequestreview-1999493861)
ACK 4722b7c7154e6130d4de66f7aed0fffe3c7c19a4, I have verified the subtree update and reviewed the build system changes. Both look OK.

✅ fanquake closed an issue: "test: intermittent issue in interface_rest.py - AssertionError: self.wait_until(lambda: self.nodes[0].getindexinfo() == expected_filter)"
(https://github.com/bitcoin/bitcoin/issues/29863)

💬 hebasto commented on pull request "build: Fix false positive `CHECK_ATOMIC` test for clang-15":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2053973941)
> The same error happens with clang-18 locally, but why does CI pass?
>
> [fad23a0](https://github.com/bitcoin/bitcoin/commit/fad23a06469607689c4f637bb407c96af4902a27)

Depends on standard C++ library version?

💬 laanwj commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-2053981804)
Concept ACK, i think it's important that signet gets its own launch icon if testnet has, it's more useful for testing than testnet. The other option would be to keep only the mainnet icon. But i think there's value in making windows users aware of these kind of things (so they don't reinvent wheels, or, test on mainnet 😅 ).

💬 fanquake commented on pull request "ci: use Clang 16 for Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29848#discussion_r1564592864)
Added

💬 laanwj commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2053992273)
Looks like it's resolved? As a rare issue with a dependency that's going away anyhow, i think this can be closed.

💬 laanwj commented on pull request "depends: suggest GNU patch for macOS <= 13":
(https://github.com/bitcoin/bitcoin/pull/29814#issuecomment-2053992693)
i would prefer not to require (nor suggest) any GNU tools on non-GNU operating systems, does this potentially affect any other deps besides bdb?

ACK on the way this is worded, anyhow. But we might just sit this out until bdb is gone.

🤔 maflcko reviewed a pull request: "depends: suggest GNU patch for macOS <= 13"
(https://github.com/bitcoin/bitcoin/pull/29814#pullrequestreview-1999542526)
Not sure. Anyone else on any version of macOS can build fine. So far only a single person ran into this issue, without steps to reproduce, so I am not sure about adjusting the docs.

💬 maflcko commented on pull request "depends: suggest GNU patch for macOS <= 13":
(https://github.com/bitcoin/bitcoin/pull/29814#discussion_r1564613112)
Installing it would be insufficient anyway, because you'd still need to overwrite the global default?

💬 maflcko commented on pull request "ci: use Clang 16 for Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29848#issuecomment-2053999845)
lgtm ACK ad21f2294821d7c436e58a8f199fb555b11a56ad

💬 maflcko commented on pull request "build: Fix false positive `CHECK_ATOMIC` test for clang-15":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2054000372)
> > The same error happens with clang-18 locally, but why does CI pass?
> > [fad23a0](https://github.com/bitcoin/bitcoin/commit/fad23a06469607689c4f637bb407c96af4902a27)
>
> Depends on standard C++ library version?

I can re-try, but my findings didn't indicate that IIRC.

In any case, the title needs to be adjusted to remove the `clang-15`, no?

💬 hebasto commented on pull request "build: Fix false positive `CHECK_ATOMIC` test":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2054000744)
> In any case, the title needs to be adjusted to remove the `clang-15`, no?

Updated.

💬 maflcko commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2054000903)
Is this related to bbe82c116e72ca0638751e063bf564cd1fe5c4d5?

💬 furszy commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2054064278)
> Is this related to [bbe82c1](https://github.com/bitcoin/bitcoin/commit/bbe82c116e72ca0638751e063bf564cd1fe5c4d5)?

No, it is related to 0faafb57f8298547949cbc0044ee9e925ed887ba. This change is partially reverting it and documenting the flow so the regression does not happen again.