💬 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
...
📝 w0xlt converted_to_draft 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
...