💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1809264269)
Rebased over #28207
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1809264269)
Rebased over #28207
💬 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_r1391795346)
It isn't. When `FundTransaction` extracts the inputs and outputs from `CreateTransction`'s result and puts them into the tx it got, so the version ends up being preserved. But `CreateTransaction` isn't aware of the version selected by the user.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391795346)
It isn't. When `FundTransaction` extracts the inputs and outputs from `CreateTransction`'s result and puts them into the tx it got, so the version ends up being preserved. But `CreateTransaction` isn't aware of the version selected by the user.
💬 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_r1391795660)
With miniscript, it does matter since scripts can contain locktimes.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391795660)
With miniscript, it does matter since scripts can contain locktimes.
💬 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_r1391795950)
With miniscript, it does matter, as it controls whether something with `OP_CSV` is spendable.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391795950)
With miniscript, it does matter, as it controls whether something with `OP_CSV` is spendable.
💬 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-1809279093)
> `0x` is much more standard and recognisable than `[]`.
It's pretty misleading though, since `CScriptNum` is little-endian: if you see `0x0100` you would expect that to be 256, but for us it would actually be non-minimally-encoded 1; For us, `0x0201` would actually be less than `0x0102`, etc.
Note this applies to block hashes, as well, so if you provide block 816651's header and execute HASH256, you end up with `[7d6368795d9675a198bd1dbe3d87dbdbbc3e0f3560a202000000000000000000]`; if you w
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1809279093)
> `0x` is much more standard and recognisable than `[]`.
It's pretty misleading though, since `CScriptNum` is little-endian: if you see `0x0100` you would expect that to be 256, but for us it would actually be non-minimally-encoded 1; For us, `0x0201` would actually be less than `0x0102`, etc.
Note this applies to block hashes, as well, so if you provide block 816651's header and execute HASH256, you end up with `[7d6368795d9675a198bd1dbe3d87dbdbbc3e0f3560a202000000000000000000]`; if you w
...
💬 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_r1391800163)
I'd prefer to leave it as is since it follows the pattern established by the following commits where various things are `Set`.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391800163)
I'd prefer to leave it as is since it follows the pattern established by the following commits where various things are `Set`.
💬 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_r1391802277)
It is actually expected that both can be provided, and not match. We may be funding a transaction with other inputs that already have some signatures (e.g. a multisig where one of the parties is adding another input for fees, and one other party has already signed with sighash_single), so `m_script_sig` or `m_script_witness` may already exist, but are incomplete. `m_weight` may then also be provided, and it wouldn't match since it would account for the signatures that have not yet been added.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1391802277)
It is actually expected that both can be provided, and not match. We may be funding a transaction with other inputs that already have some signatures (e.g. a multisig where one of the parties is adding another input for fees, and one other party has already signed with sighash_single), so `m_script_sig` or `m_script_witness` may already exist, but are incomplete. `m_weight` may then also be provided, and it wouldn't match since it would account for the signatures that have not yet been added.
💬 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:
(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.
(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.
(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);
...
(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.
(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
...
(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
(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
(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
(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
...
(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.
(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
(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`
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1391833501)
Added `Assume`