💬 S3RK commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2106892196)
Here are my initial thoughts on each of the options individually
`change_target`
IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using `change_position`parameter. The downside compared to a new option is that we always have a change output. Not sure if that's a problem in your case.
`disable_algos`
This makes sense, and could be a fir
...
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2106892196)
Here are my initial thoughts on each of the options individually
`change_target`
IIUC you want a specific min value for the change UTXO. Would a following alternative work? You provide your change address in the list of recipients with desired min amount and specify it as change UTXO using `change_position`parameter. The downside compared to a new option is that we always have a change output. Not sure if that's a problem in your case.
`disable_algos`
This makes sense, and could be a fir
...
💬 TheCharlatan commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1598055160)
Adding something to exercise `Erase` would also be nice.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1598055160)
Adding something to exercise `Erase` would also be nice.
👍 TheCharlatan approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-2051980268)
ACK ffee43efe845cbbfbf16d5e61a1d541cb316ef56
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-2051980268)
ACK ffee43efe845cbbfbf16d5e61a1d541cb316ef56
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598073431)
Done.
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598073431)
Done.
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598077078)
Would prefer to keep this PR focused on the `limit` and `CHECKSUM_SIZE` change, but perhaps a good change for #29607
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598077078)
Would prefer to keep this PR focused on the `limit` and `CHECKSUM_SIZE` change, but perhaps a good change for #29607
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598077219)
Done.
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598077219)
Done.
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598078613)
Yes, this is checking that "The data part, which is at least 6 characters long" (from BIP173). Changed `pos + 7 >` to `pos + CHECKSUM_SIZE >=` throughout.
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1598078613)
Yes, this is checking that "The data part, which is at least 6 characters long" (from BIP173). Changed `pos + 7 >` to `pos + CHECKSUM_SIZE >=` throughout.
💬 0xB10C commented on pull request "[DO NOT MERGE] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2106968575)
@willcl-ark and I ran a IBD benchmark vs master for this: IBD from a local node to height 840000 and currently only looking at the connect_block duration.
In the current draft implementation:
- master is currently faster
- batch validation is attempted even before taproot activated
- after taproot activation, batch validation is significantly slower than non-batch validation
@willcl-ark and I ran a IBD benchmark vs master for this: IBD from a local node to height 840000 and currently only looking at the connect_block duration.
In the current draft implementation:
- master is currently faster
- batch validation is attempted even before taproot activated
- after taproot activation, batch validation is significantly slower than non-batch validation
![image](htt
...
💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2106970721)
Changed the limit name to BECH32 per @sipa 's suggestion and updated to use `CHECKSUM_SIZE` throughout per @paplorinc 's review.
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2106970721)
Changed the limit name to BECH32 per @sipa 's suggestion and updated to use `CHECKSUM_SIZE` throughout per @paplorinc 's review.
💬 Saraeutsza commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2106972758)
I need help I'm not a programmer and don't understand the language.
รับ Outlook สำหรับ Android<https://aka.ms/AAb9ysg>
________________________________
From: S3RK ***@***.***>
Sent: Monday, May 13, 2024 2:55:44 PM
To: bitcoin/bitcoin ***@***.***>
Cc: Sarawut Kaewyana ***@***.***>; Comment ***@***.***>
Subject: Re: [bitcoin/bitcoin] wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees (PR #30080)
Here are
...
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2106972758)
I need help I'm not a programmer and don't understand the language.
รับ Outlook สำหรับ Android<https://aka.ms/AAb9ysg>
________________________________
From: S3RK ***@***.***>
Sent: Monday, May 13, 2024 2:55:44 PM
To: bitcoin/bitcoin ***@***.***>
Cc: Sarawut Kaewyana ***@***.***>; Comment ***@***.***>
Subject: Re: [bitcoin/bitcoin] wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees (PR #30080)
Here are
...
💬 willcl-ark commented on pull request "[DO NOT MERGE] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2106978586)
> batch validation is attempted and decreases performance even before taproot activated
From a quick skim of the changes ISTM that we always use the `BatchingCachingTransactionSignatureChecker`, and there was no switch to the `CachingTransactionSignatureChecker`, but this could well be because this is only in WIP state. It doesnt account for why it's currently slower in all cases though.
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2106978586)
> batch validation is attempted and decreases performance even before taproot activated
From a quick skim of the changes ISTM that we always use the `BatchingCachingTransactionSignatureChecker`, and there was no switch to the `CachingTransactionSignatureChecker`, but this could well be because this is only in WIP state. It doesnt account for why it's currently slower in all cases though.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598090674)
I just checked two things:
1. LevelDB does not support indexes. It also does not support SQL queries.
If we were to store the amount (of the largest taproot output) in the index, any value filtering has to be done at the time we query the index. Worst case 9000 entries per block (a 1 input 1 output taproot transaction is 111 vbyte). That might be fine, you'd have to benchmark.
If it _is_ a bottleneck, then we could refactor the base index to allow sqlite3 as a backend.
2. The only wa
...
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598090674)
I just checked two things:
1. LevelDB does not support indexes. It also does not support SQL queries.
If we were to store the amount (of the largest taproot output) in the index, any value filtering has to be done at the time we query the index. Worst case 9000 entries per block (a 1 input 1 output taproot transaction is 111 vbyte). That might be fine, you'd have to benchmark.
If it _is_ a bottleneck, then we could refactor the base index to allow sqlite3 as a backend.
2. The only wa
...
💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598091393)
I don't see the value of running a test like this? The code here is not the definition of `H`, just one way of calculating the value, based on the definition in BIP341. For example, if the test were to fail, that would just mean the comment is out of date, not that our value for `H` is incorrect.
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1598091393)
I don't see the value of running a test like this? The code here is not the definition of `H`, just one way of calculating the value, based on the definition in BIP341. For example, if the test were to fail, that would just mean the comment is out of date, not that our value for `H` is incorrect.
💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598094935)
Using `IsDust()` makes no different for the regular index: https://github.com/Sjors/bitcoin/commit/20aba5dfa34224432787ac450b8e9d3a66d8eea9
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1598094935)
Using `IsDust()` makes no different for the regular index: https://github.com/Sjors/bitcoin/commit/20aba5dfa34224432787ac450b8e9d3a66d8eea9
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598099088)
Trying to keep this a move-only refactor, so would prefer to leave it for now. I end up touching this code again in #28201; will make a note of this for that PR.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598099088)
Trying to keep this a move-only refactor, so would prefer to leave it for now. I end up touching this code again in #28201; will make a note of this for that PR.
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2107001990)
Fixed the Windows build error, but drafted while based on #30078.
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2107001990)
Fixed the Windows build error, but drafted while based on #30078.
📝 fanquake converted_to_draft a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723)
Based on #30078.
This picks up a change, which is a switch to building zeromq with CMake. It includes a patch with a couple changes I've upstreamed:
* https://github.com/zeromq/libzmq/pull/4668
* https://github.com/zeromq/libzmq/pull/4670
and another Windows-related change:
* https://github.com/zeromq/libzmq/pull/4678
I also noticed the CMake Windows version autodetction was broken with mingw-w64 (https://github.com/zeromq/libzmq/issues/4669), so we set the version explicitly.
(https://github.com/bitcoin/bitcoin/pull/29723)
Based on #30078.
This picks up a change, which is a switch to building zeromq with CMake. It includes a patch with a couple changes I've upstreamed:
* https://github.com/zeromq/libzmq/pull/4668
* https://github.com/zeromq/libzmq/pull/4670
and another Windows-related change:
* https://github.com/zeromq/libzmq/pull/4678
I also noticed the CMake Windows version autodetction was broken with mingw-w64 (https://github.com/zeromq/libzmq/issues/4669), so we set the version explicitly.
💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598105196)
Would prefer to not change for now to keep this a move-only commit.
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1598105196)
Would prefer to not change for now to keep this a move-only commit.
💬 fanquake commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#discussion_r1598107518)
Should remove `-DHARDENING=OFF` here.
(https://github.com/bitcoin/bitcoin/pull/29790#discussion_r1598107518)
Should remove `-DHARDENING=OFF` here.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598114179)
I don't think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1598114179)
I don't think it adds much here? We are using BOOST_CHECK to to ensure the function call succeeds before proceeding, so getting a line number failure for BOOST_CHECK seems sufficient for debugging. Also, if dealing with valid keys, this function should never fail.