🚀 achow101 merged a pull request: "tests: improve wallet multisig descriptor test and docs"
(https://github.com/bitcoin/bitcoin/pull/29154)

👍 tdb3 approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2167809049)
ACK 29d19c414c9e138c99f8de84e398528db92ff07e

Left some nits.

💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671449310)
nit: Would this be less than or equal to? (equal to in this case).

💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671443496)
nit: Would be good to make this comment a `self.log.info()` message instead.

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice
> Instead of inline comments or no test documentation at all, log the comments to the test log

💬 tdb3 commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671452079)
> ` -TODO: An ancestor of snapshot block`: isn't this already addressed when successfully loading the snapshot into node1 [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_assumeutxo.py#L310)?

nit: Would be good to be more explicit in the log message near the line (or add one more) to document for future readers what the test covers. This way if there are changes in the future, the changes don't accidentally remove case coverage.

🤔 tdb3 reviewed a pull request: "rpc, rest: Improve getblock error when only header is available"
(https://github.com/bitcoin/bitcoin/pull/30410#pullrequestreview-2167870869)
Concept ACK
Thanks for picking this up. Overall, I think adding clarity for the user is a net positive.

> I'm not completely sure if the cause is important enough to change the wording of the rpc error, that some scripts may rely on.

I agree, this may be a borderline case because `Block not found on disk` seems like it is still technically correct albeit not as descriptive. Since we're not changing RPC data structure or error codes (just the message), I'm not even sure if implementing `
...

💬 tdb3 commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1671493406)
nit: The comment below should also be adjusted. The case where the header is available but we don't have the block yet is covered by this new `JSONRPCError`.

// Block not found on disk. This could be because we have the block
// header in our index but not yet have the block or did not accept the
// block. Or if the block was pruned right after we released the lock above.

💬 tdb3 commented on pull request "rpc, rest: Improve getblock error when only header is available":
(https://github.com/bitcoin/bitcoin/pull/30410#discussion_r1671495550)
nit: Same for here

// Block not found on disk. This could be because we have the block
// header in our index but not yet have the block or did not accept the
// block. Or if the block was pruned right after we released the lock above.

💬 pythcoiner commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2219635092)
note: overlapping multipath are allowed
```python
self.test_importdesc({"desc": descsum_create(f"wsh(and_v(v:pk({xpriv}/<0;1>/*),or_d(pk({xpriv}/<1;2>/*),older(1000))))"),
"active": True,
"range": 10,
"timestamp": "now"},
success=True,
wallet=w_multipath)
```
but I don't see how it can be harmfull as it ends up in valid miniscript
...

💬 pythcoiner commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2219684402)
importing a descriptor w/ `"internal": false` fail because checks [here](https://github.com/bitcoin/bitcoin/blob/00337ef0e115f8bd8c1ede425f21ae7a1b6d30de/src/wallet/rpc/backup.cpp#L1073) and [here](https://github.com/bitcoin/bitcoin/blob/00337ef0e115f8bd8c1ede425f21ae7a1b6d30de/src/wallet/rpc/backup.cpp#L1476) are against if the key exist, not against its value.

```shell
~/cpp/bitcoin/src pr22838 *1 ?3 ❯ bitcoin-cli -regtest -rpcwallet=liana_recovery importdescriptors "[{\"desc\": \"wsh(or_d
...

👋 maflcko's pull request is ready for review: "util: Catch translation string errors at compile time"
(https://github.com/bitcoin/bitcoin/pull/30383)

💬 maflcko commented on pull request "random: add benchmarks and drop unnecessary Shuffle function":
(https://github.com/bitcoin/bitcoin/pull/30396#issuecomment-2219886162)
For fuzzing, just to keep it in mind, there is (obviously) no guarantee by the standard as to how `std::shuffle` behaves. See also https://en.cppreference.com/w/cpp/algorithm/random_shuffle#Notes

> Note that the implementation is not dictated by the standard, so even if you use exactly the same RandomFunc or URBG (Uniform Random Number Generator) you may get different results with different standard library implementations.

So going forward, I guess one can only assume to surely reproduce
...

⚠️ fanquake opened an issue: "ci: failure in `p2p_v2_misbehaving.py`"
(https://github.com/bitcoin/bitcoin/issues/30419)
https://github.com/bitcoin/bitcoin/actions/runs/9864841219/job/27240616054?pr=30410#step:7:5422
```bash
node0 2024-07-09T23:09:39.685000Z [net] [net.cpp:1814] [CreateNodeFromAcceptedSocket] [net] connection from 127.0.0.1:53666 accepted
test 2024-07-09T23:09:39.691000Z TestFramework (INFO): Sending remaining ellswift and garbage which are different from V1_PREFIX. Since a response is
test 2024-07-09T23:09:39.691000Z TestFramework (INFO): expected now, our custom data_received() funct
...

💬 fanquake commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2219908962)
Looks like this is causing intermittent CI failures. See #30419.

💬 fanquake commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2219919953)
Seen recently here (`wallet_bumpfee.py --descriptors`) https://github.com/bitcoin/bitcoin/actions/runs/9863555844/job/27236587622#step:27:12496:
```bash
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_node.py", line 188, in _raise_assertion_error
raise AssertionError(self._node_msg(msg))
AssertionError: [node 1] Unable to connect to bitcoind after 2400s
test 2024-0
...

💬 maflcko commented on pull request "util: Catch translation string errors at compile time":
(https://github.com/bitcoin/bitcoin/pull/30383#issuecomment-2219940911)
Rebased to drop merged commit.

Current CI failure is unrelated and can be ignored.

💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897509)
I think it's a bit too verbose for the log but I added a comment. And I think it will be confusing to explicitly say everywhere in the normal case that the tip is an ancestor of the snapshot. But it's documented in one place now at least.

💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897648)
I think it's fine to keep the function name as is. The comment (now a log) provides the necessary context and I think it's just a special edge case of the less work behavior.

💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#discussion_r1671897825)
Done

💬 fjahr commented on pull request "chainparams: Change nChainTx type to uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29656#issuecomment-2219987968)
Rebased (caused by #30329)