💬 hebasto commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2053737139)
Concept ACK.

💬 hebasto commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2053738677)
> remove unused cpp-subprocess options

Are all remaining options used?

🤔 vostrnad reviewed a pull request: "[27.x] More backports and finalize"
(https://github.com/bitcoin/bitcoin/pull/29780#pullrequestreview-1999340097)
Reviewed the release notes. A few style nits/suggestions, one entry is missing its PR number.

💬 vostrnad commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564218367)
```suggestion
- The `mempool.dat` file created by `-persistmempool` or the savemempool RPC will
```

💬 vostrnad commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564218402)
```suggestion
- Opt-in Topologically Restricted Until Confirmation (TRUC) Transactions policy
```

💬 vostrnad commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564218594)
```suggestion
flush in order to speed up the syncing of such nodes. (#20827)
```

💬 vostrnad commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564218569)
```suggestion
- The addnode RPC now follows the `-v2transport` option (now on by default, see above) for making connections.
```

💬 vostrnad commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564218615)
```suggestion
- Improved handling of empty `settings.json` files. (#28920)
```

💬 vostrnad commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564218543)
```suggestion
(aka v3 transaction policy) is available for use on test networks when
`-acceptnonstdtxn=1` is set. By setting the transaction version number to 3, TRUC transactions
```

💬 okorye commented on pull request "[27.x] More backports and finalize":
(https://github.com/bitcoin/bitcoin/pull/29780#discussion_r1564292710)
- [0x2ed4De9a4c3D09A8917E4CE31dB3abE473de87a4https://t.me/Cryptocom_DeFiWallet/236016`{"SPDXID":"SPDXRef-DOCUMENT","spdxVersion":"SPDX-2.3","creationInfo":{"created":"202403-05T10:18:00Z","creators":["Tool: GitHub.com-DependencyGraph"]},"name":"com.github.okorye/codespaces-express","dataLicense":"CC01.0","documentDescribes":["SPDXRef-com.github.okorye-codespacesexpress"],"documentNamespace":"https://github.com/okorye/codespaces-
express/dependency_graph/sbom-ce3fd3ce36630cb4","packages":[{"SPDXI
...

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1564307022)
Good point! I added the message like you suggest. Logs now read:
```
2024-04-13T23:05:41.654000Z TestFramework (INFO): At block height >= 128 this multisig is 3-of-4
2024-04-13T23:05:41.674000Z TestFramework (INFO): Check that the time-locked transaction is too immature to spend with 3-of-4 at block height 103...
2024-04-13T23:05:41.675000Z TestFramework (INFO): Generate blocks to reach the time-lock block height 128 and broadcast the transaction...
2024-04-13T23:05:42.888000Z TestFramework
...

🤔 tdb3 reviewed a pull request: "test: Validate transaction without inputs"
(https://github.com/bitcoin/bitcoin/pull/29862#pullrequestreview-1999388825)
ACK for 1240610fcf0651f217fd01de387e1047dc18485f

Great work adding more coverage.
Built and ran all unit tests (including `transaction_tests`). Passed.

💬 tdb3 commented on pull request "test: Validate transaction without inputs":
(https://github.com/bitcoin/bitcoin/pull/29862#discussion_r1564345633)
To check that unexpected reject reasons would cause test failure, added characters to the end of the reject reason check. Test failed as expected.

```c
BOOST_CHECK(state.GetRejectReason() == "bad-txns-vin-empty_ShouldCauseTestFailure");
```

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1564361740)
I just got confused here. CLTV opcode checks against spending transaction `nLockTime`. So behavior is exactly as expected

> Do you mean the 4-signature transaction was rejected?

No I didn't mean this. 4-signature transaction would never be rejected. But I explained this confusingly so you can ignore my comment above

💬 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
...