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

👍 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 😅 ).