💬 maflcko commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3060581268)
needs release note?
(https://github.com/bitcoin/bitcoin/pull/32944#issuecomment-3060581268)
needs release note?
💬 Sjors commented on issue "Guix build fails on M4 macOS host with Ubuntu in UTM":
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3060808833)
The new machine also gives me the "cannot set armhf-linux personality" error.
It seems to happen after successfully building one package (with `--max-jobs=1`). Each time you do `guix build` it'll build another package and throw this error.
If I switch back to c8abd972818f83d90bf50b250131f338034460ef then `guix build` just works. At least after I fix the app armor issue:
In `/etc/apparmor.d/guix`:
```
abi <abi/4.0>,
include <tunables/global>
# Profile for the guix binary
profile guix /usr/bi
...
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3060808833)
The new machine also gives me the "cannot set armhf-linux personality" error.
It seems to happen after successfully building one package (with `--max-jobs=1`). Each time you do `guix build` it'll build another package and throw this error.
If I switch back to c8abd972818f83d90bf50b250131f338034460ef then `guix build` just works. At least after I fix the app armor issue:
In `/etc/apparmor.d/guix`:
```
abi <abi/4.0>,
include <tunables/global>
# Profile for the guix binary
profile guix /usr/bi
...
💬 Prabhat1308 commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3060846745)
closing in favour of #32944
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3060846745)
closing in favour of #32944
💬 Prabhat1308 commented on pull request "test: add functional test for upgradewallet rpc":
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3060848294)
closing in favour of #32944
(https://github.com/bitcoin/bitcoin/pull/32803#issuecomment-3060848294)
closing in favour of #32944
✅ Prabhat1308 closed a pull request: "test: add functional test for upgradewallet rpc"
(https://github.com/bitcoin/bitcoin/pull/32803)
(https://github.com/bitcoin/bitcoin/pull/32803)
💬 Sjors commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2199811385)
765922d8022d3addeb80b5f3f6e041f7fd2ad3ed: do you know why https://codeberg.org/guix/guix/pulls/353/files only needs `python-pydantic-2` and not `python-pydantic-core`?
(https://github.com/bitcoin/bitcoin/pull/32431#discussion_r2199811385)
765922d8022d3addeb80b5f3f6e041f7fd2ad3ed: do you know why https://codeberg.org/guix/guix/pulls/353/files only needs `python-pydantic-2` and not `python-pydantic-core`?
🤔 Prabhat1308 reviewed a pull request: "wallet: Remove `upgradewallet` RPC"
(https://github.com/bitcoin/bitcoin/pull/32944#pullrequestreview-3008894533)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32944#pullrequestreview-3008894533)
Concept ACK
💬 Prabhat1308 commented on pull request "wallet: Remove `upgradewallet` RPC":
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2199814332)
There is another instance of `SetMinVersion` in this file . Since `Createwallet` will create a wallet with the newest version , that use is also a dead code . Might as well remove this function too.
(https://github.com/bitcoin/bitcoin/pull/32944#discussion_r2199814332)
There is another instance of `SetMinVersion` in this file . Since `Createwallet` will create a wallet with the newest version , that use is also a dead code . Might as well remove this function too.
💬 Sjors commented on issue "Guix build fails on M4 macOS host with Ubuntu in UTM":
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3060956972)
Perhaps an easier thing for me to test would be if switching to guix own python-lief 0.16.6 package magically fixes things. This would require a time machine bump past: https://codeberg.org/guix/guix/pulls/353/files
@fanquake in https://github.com/bitcoin/bitcoin/pull/32764#pullrequestreview-2942922469 you mentioned having a time machine bump brach?
Meanwhile I'll try if the simple act of adding either `python-scikit-build-core`, `python-pydantic-2` or `python-pydantic-core` on top of https://
...
(https://github.com/bitcoin/bitcoin/issues/32759#issuecomment-3060956972)
Perhaps an easier thing for me to test would be if switching to guix own python-lief 0.16.6 package magically fixes things. This would require a time machine bump past: https://codeberg.org/guix/guix/pulls/353/files
@fanquake in https://github.com/bitcoin/bitcoin/pull/32764#pullrequestreview-2942922469 you mentioned having a time machine bump brach?
Meanwhile I'll try if the simple act of adding either `python-scikit-build-core`, `python-pydantic-2` or `python-pydantic-core` on top of https://
...
💬 Sjors commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061031655)
utACK f43571010e3853e83a21aa4774b1c8da47b5d961
x86_64 guix hashes:
```
bb9e6435df82cfe01c78e046ef39cea62a3b0ad0deafa66e1d2a15257bc21eeb guix-build-f43571010e38/output/dist-archive/bitcoin-f43571010e38.tar.gz
7d3bc9459a51abcb2d9d36b69fc135cbb4940c092a9f8df63032c6e9f659a958 guix-build-f43571010e38/output/x86_64-w64-mingw32/SHA256SUMS.part
34af63c893a315745364d1f4dd36bbc1521eef0cad42ea964ba1d0dc5cbcf738 guix-build-f43571010e38/output/x86_64-w64-mingw32/bitcoin-f43571010e38-win64-codesig
...
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061031655)
utACK f43571010e3853e83a21aa4774b1c8da47b5d961
x86_64 guix hashes:
```
bb9e6435df82cfe01c78e046ef39cea62a3b0ad0deafa66e1d2a15257bc21eeb guix-build-f43571010e38/output/dist-archive/bitcoin-f43571010e38.tar.gz
7d3bc9459a51abcb2d9d36b69fc135cbb4940c092a9f8df63032c6e9f659a958 guix-build-f43571010e38/output/x86_64-w64-mingw32/SHA256SUMS.part
34af63c893a315745364d1f4dd36bbc1521eef0cad42ea964ba1d0dc5cbcf738 guix-build-f43571010e38/output/x86_64-w64-mingw32/bitcoin-f43571010e38-win64-codesig
...
💬 maflcko commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061186975)
lgtm ACK f43571010e3853e83a21aa4774b1c8da47b5d961
Haven't done a guix build, because my riscv64 machine died. I'll try to set it up fresh again next week, or so.
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061186975)
lgtm ACK f43571010e3853e83a21aa4774b1c8da47b5d961
Haven't done a guix build, because my riscv64 machine died. I'll try to set it up fresh again next week, or so.
📝 ajtowns opened a pull request: "tests: speed up coins_tests by parallelizing"
(https://github.com/bitcoin/bitcoin/pull/32945)
Updates the cmake logic to generate a separate test for each BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp into three separate suites so that they can be run in parallel.
(https://github.com/bitcoin/bitcoin/pull/32945)
Updates the cmake logic to generate a separate test for each BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp into three separate suites so that they can be run in parallel.
💬 ajtowns commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3061291677)
For me, coins_tests it's a pretty significant bottleneck to running tests. With this patch and -j20 I go from:
```
137/139 Test #26: checkqueue_tests ..................... Passed 50.37 sec
138/139 Test #126: coinselector_tests ................... Passed 52.53 sec
139/139 Test #28: coins_tests .......................... Passed 137.43 sec
100% tests passed, 0 tests failed out of 139
Total Test time (real) = 137.43 sec
```
to
```
69/141 Test #30: coins_tests .....
...
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3061291677)
For me, coins_tests it's a pretty significant bottleneck to running tests. With this patch and -j20 I go from:
```
137/139 Test #26: checkqueue_tests ..................... Passed 50.37 sec
138/139 Test #126: coinselector_tests ................... Passed 52.53 sec
139/139 Test #28: coins_tests .......................... Passed 137.43 sec
100% tests passed, 0 tests failed out of 139
Total Test time (real) = 137.43 sec
```
to
```
69/141 Test #30: coins_tests .....
...
💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200122553)
In d94c02e109617a7a4d31a5f03d6f5221f2faf878 "refactor: use SignOptions for MutableTransactionSignatureCreator"
Possible to use `SIGHASH_ALL` here instead of `1` now?
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200122553)
In d94c02e109617a7a4d31a5f03d6f5221f2faf878 "refactor: use SignOptions for MutableTransactionSignatureCreator"
Possible to use `SIGHASH_ALL` here instead of `1` now?
💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200043048)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
ISTM that the default value of `bip32derivs` is switched from `false` to `true` with the use of `PSBTFillOptions` here. Is this intended?
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200043048)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
ISTM that the default value of `bip32derivs` is switched from `false` to `true` with the use of `PSBTFillOptions` here. Is this intended?
💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200086138)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
This also does the same change for `bip32derivs` default value in SPKMs. Intended?
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200086138)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
This also does the same change for `bip32derivs` default value in SPKMs. Intended?
🤔 rkrux reviewed a pull request: "refactor: use options struct for signing and PSBT operations"
(https://github.com/bitcoin/bitcoin/pull/32876#pullrequestreview-3009152210)
Code review d94c02e109617a7a4d31a5f03d6f5221f2faf878
Mostly lgtm, asked couple questions along with couple nits.
Using `SIGHASH_DEFAULT` as the default value in this PR instead of relying on `int sighash` being `0` as done previously is much better from readability POV.
(https://github.com/bitcoin/bitcoin/pull/32876#pullrequestreview-3009152210)
Code review d94c02e109617a7a4d31a5f03d6f5221f2faf878
Mostly lgtm, asked couple questions along with couple nits.
Using `SIGHASH_DEFAULT` as the default value in this PR instead of relying on `int sighash` being `0` as done previously is much better from readability POV.
💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200027956)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
Opinionated supernit if retouched: For variables that are supposed to take action, names starting with a verb is easier to relate to imho. Eg: `sign` & `finalize` in this struct.
So, maybe `derive_bip32.`
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2200027956)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
Opinionated supernit if retouched: For variables that are supposed to take action, names starting with a verb is easier to relate to imho. Eg: `sign` & `finalize` in this struct.
So, maybe `derive_bip32.`
💬 rkrux commented on pull request "refactor: use options struct for signing and PSBT operations":
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2199999001)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
Nit if retouched:
```diff
- if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
+ if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, /*options=*/{}) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
```
(https://github.com/bitcoin/bitcoin/pull/32876#discussion_r2199999001)
In 576da4c7dd83f7ab871bac9d76685d112e60e4ab "refactor: use PSBTFillOptions for filling and signing"
Nit if retouched:
```diff
- if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, std::nullopt) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
+ if (SignPSBTInput(DUMMY_SIGNING_PROVIDER, psbtx, i, nullptr, /*options=*/{}) != PSBTError::OK || !psbtx.GetInputUTXO(newcoin.out, i)) {
```
💬 fanquake commented on pull request "Resolve guix non-determinism with emplace_back instead of push_back":
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061439683)
Guix Build (aarch64):
```bash
b75d75ed9b64aadadcccce97bf1fcafa2802c916f4a12befe7fef3260f2d5bb7 guix-build-f43571010e38/output/aarch64-linux-gnu/SHA256SUMS.part
a9cdcb840b05cad7c69dafb13003ff2e051ab162e30a8959af011f76c5abe576 guix-build-f43571010e38/output/aarch64-linux-gnu/bitcoin-f43571010e38-aarch64-linux-gnu-debug.tar.gz
7269ff5420cd864c351fc075c0cd8b57c9f73264dd5083eabeddf5f673a88263 guix-build-f43571010e38/output/aarch64-linux-gnu/bitcoin-f43571010e38-aarch64-linux-gnu.tar.gz
8c169b
...
(https://github.com/bitcoin/bitcoin/pull/32930#issuecomment-3061439683)
Guix Build (aarch64):
```bash
b75d75ed9b64aadadcccce97bf1fcafa2802c916f4a12befe7fef3260f2d5bb7 guix-build-f43571010e38/output/aarch64-linux-gnu/SHA256SUMS.part
a9cdcb840b05cad7c69dafb13003ff2e051ab162e30a8959af011f76c5abe576 guix-build-f43571010e38/output/aarch64-linux-gnu/bitcoin-f43571010e38-aarch64-linux-gnu-debug.tar.gz
7269ff5420cd864c351fc075c0cd8b57c9f73264dd5083eabeddf5f673a88263 guix-build-f43571010e38/output/aarch64-linux-gnu/bitcoin-f43571010e38-aarch64-linux-gnu.tar.gz
8c169b
...