💬 achow101 commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-2218845486)
> Do you happen to know which pull request added a new positional argument in the middle? Or was this instruction always broken? cc @achow101

Highly likely it was always broken. I'd be surprised if a PR that added a positional argument in the middle would make it through code review.



> > A future extension could add an optional return field with device capabilities. Perhaps a descriptor with wildcards. For example: ["pkh("44'/0'/$'/{0,1}/_"), sh(wpkh("49'/0'/$'/{0,1}/_")), wpkh("84'/0'
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2218849345)
Updated 49e0b42e805448423d406edc4e5ebfb34aee959b -> 692fc11f0aae3c8013f6f01d133139ce8eba7135 ([`pr/fatalresult.16`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.16) -> [`pr/fatalresult.17`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.16..pr/fatalresult.17)) to fix conflict with #30344. Also took suggestions to simplify usage in CompleteChainstateInitialization.

💬 achow101 commented on issue "Enable `importprivkey`, `addmultisigaddress` in descriptor wallets":
(https://github.com/bitcoin/bitcoin/issues/30175#issuecomment-2218850100)
> Hmm, what would happen if `addmultisigaddress` for descriptor wallets just imports the computed descriptor as it does now (with RPC arguments that must be existing addresses or hex pubkeys)? Would it support (participating in) signing for the resulting multisig address (assuming signing for one of specified addresses/pubkeys was possible prior to the RPC call)? Would `listdescriptors true` reveal the corresponding private key?

That breaks current invariants in descriptor wallets that have p
...

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#issuecomment-2218998406)
Rebased for silent merge conflict.

💬 achow101 commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#issuecomment-2219044236)
ACK d93b79470916b1e6f85c55cc6beb1e41b382196f

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