💬 murchandamus commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2232211925)
The phrasing "Test that when the max weight is exceeded, only the most valuable encountered UTXOs are returned" sounds potentially misleading to me, because it could be understood as the algorithm changing to largest-first if the max weight is exceeded or the input set getting pruned beyond dropping below the weight limit, but this only tests that a solution is found that stays within the limit.
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2232211925)
The phrasing "Test that when the max weight is exceeded, only the most valuable encountered UTXOs are returned" sounds potentially misleading to me, because it could be understood as the algorithm changing to largest-first if the max weight is exceeded or the input set getting pruned beyond dropping below the weight limit, but this only tests that a solution is found that stays within the limit.
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232213129)
This is indeed a lot nicer, thanks!
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232213129)
This is indeed a lot nicer, thanks!
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232216843)
I quickly pushed a rebase, thinking I'll follow with applying the recommendations - but ended up in a `CompressedScript` rabbithole (which I decided to leave to a follow-up in the end).
Looks like the extractions and inlines I did here ended up with lots of unnecessary casts, thanks for noticing, cleaned it up in latest push!
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232216843)
I quickly pushed a rebase, thinking I'll follow with applying the recommendations - but ended up in a `CompressedScript` rabbithole (which I decided to leave to a follow-up in the end).
Looks like the extractions and inlines I did here ended up with lots of unnecessary casts, thanks for noticing, cleaned it up in latest push!
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3120688624)
Thanks for the recommendations, took them and extended the tests slightly to document the current behavior of `CompressedScript` (unchanged) and uncompressed P2PK as well (also unchanged).
I have also rebased to make sure all new test cases are run - ready for re-review.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3120688624)
Thanks for the recommendations, took them and extended the tests slightly to document the current behavior of `CompressedScript` (unchanged) and uncompressed P2PK as well (also unchanged).
I have also rebased to make sure all new test cases are run - ready for re-review.
💬 achow101 commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3120690351)
I'm not really a fan of changing binary locations yet again. If this is merged for v30.0, we'll have the 3 active major release branches all with different binary locations. I expect this will make my workflow a bit harder and make backports more annoying to test.
Mainly my complaint is about the locations of `bench_bitcoin` and `test_bitcoin` in builds from source since those end up being used quite a lot in my development workflow.
For the other (new) binaries that would be in libexec, I
...
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-3120690351)
I'm not really a fan of changing binary locations yet again. If this is merged for v30.0, we'll have the 3 active major release branches all with different binary locations. I expect this will make my workflow a bit harder and make backports more annoying to test.
Mainly my complaint is about the locations of `bench_bitcoin` and `test_bitcoin` in builds from source since those end up being used quite a lot in my development workflow.
For the other (new) binaries that would be in libexec, I
...
🤔 naiyoma reviewed a pull request: "util: Revert "common: Close non-std fds before exec in RunCommandJSON""
(https://github.com/bitcoin/bitcoin/pull/33063#pullrequestreview-3057157243)
Concept ACK
`
From POSIX.1-2017 fork() documentation:
'A process shall be created with a single thread. If a multi-threaded process calls fork(), the new process shall contain a replica of the calling thread and its entire address space, possibly including the states of mutexes and other resources. Consequently, to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called.'
`
(https://github.com/bitcoin/bitcoin/pull/33063#pullrequestreview-3057157243)
Concept ACK
`
From POSIX.1-2017 fork() documentation:
'A process shall be created with a single thread. If a multi-threaded process calls fork(), the new process shall contain a replica of the calling thread and its entire address space, possibly including the states of mutexes and other resources. Consequently, to avoid errors, the child process may only execute async-signal-safe operations until such time as one of the exec functions is called.'
`
🤔 dr8931240-ux reviewed a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3057166764)
Thank you
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3057166764)
Thank you
💬 l0rinc commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3120795667)
Post-merge ACK
It seems the post-merge build was partially cancelled for some reason, but the follow-ups passed fully 👍
(https://github.com/bitcoin/bitcoin/pull/32967#issuecomment-3120795667)
Post-merge ACK
It seems the post-merge build was partially cancelled for some reason, but the follow-ups passed fully 👍
📝 naiyoma opened a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067)
This PR improves mempool_accept_wtxid.py by:
1. Using a pre-mined chain instead of generating new blocks 522bf76d7d8058872a008a721831da264881746d
2. Using MiniWallet to avoid RPC calls for signing transactions 4ec5ae9fe126ffabcd429277092e3b27f483d430
3. Removing child txid variables and using txid.hex directly 361ebd56eb817b1e054ee1aacc87bf60c06c6b94
(https://github.com/bitcoin/bitcoin/pull/33067)
This PR improves mempool_accept_wtxid.py by:
1. Using a pre-mined chain instead of generating new blocks 522bf76d7d8058872a008a721831da264881746d
2. Using MiniWallet to avoid RPC calls for signing transactions 4ec5ae9fe126ffabcd429277092e3b27f483d430
3. Removing child txid variables and using txid.hex directly 361ebd56eb817b1e054ee1aacc87bf60c06c6b94
🤔 dr8931240-ux reviewed a pull request: "truc: optimize the in package relation calculation"
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3057214081)
Ok
(https://github.com/bitcoin/bitcoin/pull/33062#pullrequestreview-3057214081)
Ok
💬 l0rinc commented on pull request "test: Move `script_assets_tests` into its own suite":
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-3120838340)
test-only post-merge ACK
It's reporting `script_assets_tests` as skipped on Mac
<details>
<summary>Details</summary>
```bash
129/144 Test #86: script_assets_tests ..................***Skipped 0.36 sec
...
The following tests did not run:
86 - script_assets_tests (Skipped)
```
</details>
and on Linux
<details>
<summary>Details</summary>
```bash
139/144 Test #86: script_assets_tests ..................***Skipped 0.20 sec
...
The following tests did not
...
(https://github.com/bitcoin/bitcoin/pull/31576#issuecomment-3120838340)
test-only post-merge ACK
It's reporting `script_assets_tests` as skipped on Mac
<details>
<summary>Details</summary>
```bash
129/144 Test #86: script_assets_tests ..................***Skipped 0.36 sec
...
The following tests did not run:
86 - script_assets_tests (Skipped)
```
</details>
and on Linux
<details>
<summary>Details</summary>
```bash
139/144 Test #86: script_assets_tests ..................***Skipped 0.20 sec
...
The following tests did not
...
💬 l0rinc commented on pull request "ci: Run unit test parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33000#issuecomment-3120840192)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33000#issuecomment-3120840192)
Concept ACK
💬 l0rinc commented on pull request "refactor: inline constant return values from `dbwrapper` write methods":
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3120847592)
> Looks like this was introduced in https://github.com/bitcoin/bitcoin/commit/421218d3040279c84616891e8d14b05576b07c57
Maybe @sipa can help us out here: should we try making read/write symmetric (either by `write` returning, or `read` throwing) or is the current refactor the cleanest direction?
(https://github.com/bitcoin/bitcoin/pull/33042#issuecomment-3120847592)
> Looks like this was introduced in https://github.com/bitcoin/bitcoin/commit/421218d3040279c84616891e8d14b05576b07c57
Maybe @sipa can help us out here: should we try making read/write symmetric (either by `write` returning, or `read` throwing) or is the current refactor the cleanest direction?
📝 w0xlt opened a pull request: "wallet: Add address validation in `sendtoaddress` RPC"
(https://github.com/bitcoin/bitcoin/pull/33068)
Is there a specific reason not to validate the address in the `sendtoaddress` RPC?
This PR adds that.
(https://github.com/bitcoin/bitcoin/pull/33068)
Is there a specific reason not to validate the address in the `sendtoaddress` RPC?
This PR adds that.
💬 ajtowns commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232582690)
```c++
static void check_script_has_static_size(CScript& script, unsigned int size)
{
BOOST_CHECK_EQUAL(script.size(), size);
BOOST_CHECK_EQUAL(script.capacity(), CScriptBase::STATIC_SIZE);
BOOST_CHECK_EQUAL(script.allocated_memory(), 0);
}
static void check_script_has_dynamic_size(CScript& script, unsigned int size)
{
BOOST_CHECK_EQUAL(script.size(), size);
BOOST_CHECK_EQUAL(script.capacity(), size);
BOOST_CHECK_EQUAL(script.allocated_memory(), size);
}
``
...
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232582690)
```c++
static void check_script_has_static_size(CScript& script, unsigned int size)
{
BOOST_CHECK_EQUAL(script.size(), size);
BOOST_CHECK_EQUAL(script.capacity(), CScriptBase::STATIC_SIZE);
BOOST_CHECK_EQUAL(script.allocated_memory(), 0);
}
static void check_script_has_dynamic_size(CScript& script, unsigned int size)
{
BOOST_CHECK_EQUAL(script.size(), size);
BOOST_CHECK_EQUAL(script.capacity(), size);
BOOST_CHECK_EQUAL(script.allocated_memory(), size);
}
``
...
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232583047)
you think that would be easier to follow?
If it fails I would see it logged in `check_script_has_dynamic_size` - unless I make it a macro.
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232583047)
you think that would be easier to follow?
If it fails I would see it logged in `check_script_has_dynamic_size` - unless I make it a macro.
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232585256)
Thanks, pushed, let me know what you think!
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232585256)
Thanks, pushed, let me know what you think!
✅ w0xlt closed a pull request: "wallet: Add address validation in `sendtoaddress` RPC"
(https://github.com/bitcoin/bitcoin/pull/33068)
(https://github.com/bitcoin/bitcoin/pull/33068)
💬 ajtowns commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232640266)
Can get errors like:
```
test/script_tests.cpp(1246): error: in "script_tests/script_size_and_capacity_test": check script_dynamic_alloc( script, 64, 64 ) has failed for ( CScript(size=67,cap=67), 64, 64 )
test/script_tests.cpp(1253): error: in "script_tests/script_size_and_capacity_test": check script_dynamic_alloc( script, 10, 1000 ) has failed for ( CScript(size=71,cap=103), 10, 1000 )
```
with this approach:
```diff
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cp
...
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2232640266)
Can get errors like:
```
test/script_tests.cpp(1246): error: in "script_tests/script_size_and_capacity_test": check script_dynamic_alloc( script, 64, 64 ) has failed for ( CScript(size=67,cap=67), 64, 64 )
test/script_tests.cpp(1253): error: in "script_tests/script_size_and_capacity_test": check script_dynamic_alloc( script, 10, 1000 ) has failed for ( CScript(size=71,cap=103), 10, 1000 )
```
with this approach:
```diff
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cp
...
📝 w0xlt opened a pull request: "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver"
(https://github.com/bitcoin/bitcoin/pull/33069)
This PR implements [BIP-353](https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki) support in Bitcoin Core's wallet, enabling users to send Bitcoin to human-readable usernames instead of traditional addresses.
### What is BIP-353?
BIP-353 specifies a method for resolving human-friendly payment instructions via DNS. Instead of sharing long Bitcoin addresses, users can share memorable usernames like `alice._bitcoin-payment.example.com`.
### Key Features
* New `-dnsresolver` opt
...
(https://github.com/bitcoin/bitcoin/pull/33069)
This PR implements [BIP-353](https://github.com/bitcoin/bips/blob/master/bip-0353.mediawiki) support in Bitcoin Core's wallet, enabling users to send Bitcoin to human-readable usernames instead of traditional addresses.
### What is BIP-353?
BIP-353 specifies a method for resolving human-friendly payment instructions via DNS. Instead of sharing long Bitcoin addresses, users can share memorable usernames like `alice._bitcoin-payment.example.com`.
### Key Features
* New `-dnsresolver` opt
...