💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391802413)
:shrug:

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391808366)
Yes.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391811792)
No, the point is so that when we are writing an HD key, this checksum covers the entire xpub too, not just the pubkey part.

💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1809302135)
> One question I have is whether this will interfere with the scripthash types too badly? e.g. Is this nested bracket undesirable?
>
> specifically:
>
> ```shell
> [... 2dfb[ALL]]
> ```
>
> It might be preferable to have this format instead?
>
> ```shell
> [... 2dfb][ALL]
> ```

I guess it being inside the brackets seems slightly clearer to me? Could have different brackets for the two things, eg `[... 2dfb<ALL>]`. Could perhaps also drop the feature entirely (reverting #5264);
...

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391815934)
I don't think that's helpful here since we need to get a copy of the serialized xpub in order to calculate the checksum.

📝 brunoerg opened a pull request: "test: add stress test for bucketing with asmap"
(https://github.com/bitcoin/bitcoin/pull/28869)
The idea of this test is "stressing" the addrman using asmap. You should run this test using your own asmap file (`./test/functional/feature_bucketing_stress.py --asmap=path/to/asmap`).

**How it works?**

- Read the asmap file
- Get **2000** unique ASNs and their respective ranges.
- For 1/3 of the ASNs: it will try to add `1000 addresses` from each ASN into the "new" table.
- For 2/3 of the ASNs: it will try to add 1 address from each ASN into the "new" table.


I'm first opening it
...

💬 brunoerg commented on pull request "test: add stress test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1809307725)
cc: @fjahr

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1391817693)
Done as suggested

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1391817724)
Done as suggested

💬 CryptAxe commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1809311749)
> Note that BIP-301 suffers from an [equivocation attack](https://petertodd.org/2023/drivechains#equivocation-attack). While there are [many other reasons](https://petertodd.org/2023/drivechains) to NACK this pull-req, at minimum the equivocation attack is a sufficient reason alone.

If that was an issue then we would add 1 byte... as you mention yourself "An obvious solution would be to tag the OP_Return outputs with some kind of merge-mined chain identifier, and implement a strict rule conse
...

💬 achow101 commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1391827646)
used the new helper.

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833461)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833501)
Added `Assume`

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833534)
Done as suggested

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833557)
Renamed as suggested

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833583)
Dropped

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833609)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833667)
Done

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833705)
Good point, changed to two functions `GetRootPubKey` and `GetRootExtPubKey`, each returning an optional `CPubKey` or `CExtPubKey`, respectively.

💬 achow101 commented on pull request "test: Generate coverage report without running tests":
(https://github.com/bitcoin/bitcoin/pull/28772#issuecomment-1809341585)
> Is it still accurate to call the coverage report `test_bitcoin` when it may cover something else?

Any suggestions?

> Do the docs need an update?

I don't think so? (besides renaming, if we do that). The way I read the docs was that `make check` had to be run first, and separately. I hadn't expected `make cov` to run them for me.