💬 ryanofsky commented on pull request "test: avoid treating hash results as integers":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r2016550923)
re: https://github.com/bitcoin/bitcoin/pull/32050#discussion_r2015670851
> Note that both the `rehash` and `getwtxid` methods return a hex string.
Oops, sorry I misread the code. I saw `rehash` was setting the `sha256` member, and didn't realize it was actually returning a different member.
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r2016550923)
re: https://github.com/bitcoin/bitcoin/pull/32050#discussion_r2015670851
> Note that both the `rehash` and `getwtxid` methods return a hex string.
Oops, sorry I misread the code. I saw `rehash` was setting the `sha256` member, and didn't realize it was actually returning a different member.
💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2016558898)
r""-string seems more consistent. Thanks for pointing it out! Will fix if I re-touch.
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2016558898)
r""-string seems more consistent. Thanks for pointing it out! Will fix if I re-touch.
💬 laanwj commented on pull request "crypto: Use secure_allocator for `AES256_ctx`":
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2758041960)
> I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.
This may still be true, though FWIW the idea behind the `LockedPool` is that it reduces the amount of locking/unlocking operations by mapping and locking memory in blocks, not every time it's requested.
(https://github.com/bitcoin/bitcoin/pull/31774#issuecomment-2758041960)
> I think the approach here might have an unacceptable performance impact, as it is allocating and deallocating a secure area for every individual key being encrypted/decrypted.
This may still be true, though FWIW the idea behind the `LockedPool` is that it reduces the amount of locking/unlocking operations by mapping and locking memory in blocks, not every time it's requested.
💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758045480)
> I plan to retest this with v28 to try and determine if this is a router issue, or incompatibility with the (new) implementation in v29.
It appears that v28.1 does port map OK on my router:
```log
$ ./bitcoin-28.1/bin/bitcoind -natpmp=1 -port=9654 -daemon=0 | rg pmp
2025-03-27T13:14:12Z Command-line arg: natpmp="1"
2025-03-27T13:14:17Z natpmp: Port mapping successful. External address = xx.xx.xx.xx:9654
```
But v29rc1 does not:
```log
$ bitcoind -natpmp=1 -port=9654 -daemon=0 -debug=net | r
...
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758045480)
> I plan to retest this with v28 to try and determine if this is a router issue, or incompatibility with the (new) implementation in v29.
It appears that v28.1 does port map OK on my router:
```log
$ ./bitcoin-28.1/bin/bitcoind -natpmp=1 -port=9654 -daemon=0 | rg pmp
2025-03-27T13:14:12Z Command-line arg: natpmp="1"
2025-03-27T13:14:17Z natpmp: Port mapping successful. External address = xx.xx.xx.xx:9654
```
But v29rc1 does not:
```log
$ bitcoind -natpmp=1 -port=9654 -daemon=0 -debug=net | r
...
💬 laanwj commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758133629)
@willcl-ark that log is missing some parts to be able to follow what is happening, can you query for all of `natpmp:|pcp:|portmap:`? Most importantly, does the log say anything about `gateway [IPv4]`? The important part to make sure here is that it finds the default gateway and tries to connect to it (and if so, what error it gets).
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758133629)
@willcl-ark that log is missing some parts to be able to follow what is happening, can you query for all of `natpmp:|pcp:|portmap:`? Most importantly, does the log say anything about `gateway [IPv4]`? The important part to make sure here is that it finds the default gateway and tries to connect to it (and if so, what error it gets).
💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758200852)
A hopefully-redacted log:
```log
2025-03-27T13:54:42Z Bitcoin Core version v29.0.0rc1 (release build)
2025-03-27T13:54:42Z Using the 'x86_shani(1way,2way)' SHA256 implementation
2025-03-27T13:54:42Z Command-line arg: daemon="0"
2025-03-27T13:54:42Z Command-line arg: debug="net"
2025-03-27T13:54:42Z Command-line arg: natpmp="1"
2025-03-27T13:54:42Z Command-line arg: port="9654"
2025-03-27T13:54:42Z Using at most 125 automatic connections (65535 file descriptors available)
2025-03-27T13:54:42Z
...
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758200852)
A hopefully-redacted log:
```log
2025-03-27T13:54:42Z Bitcoin Core version v29.0.0rc1 (release build)
2025-03-27T13:54:42Z Using the 'x86_shani(1way,2way)' SHA256 implementation
2025-03-27T13:54:42Z Command-line arg: daemon="0"
2025-03-27T13:54:42Z Command-line arg: debug="net"
2025-03-27T13:54:42Z Command-line arg: natpmp="1"
2025-03-27T13:54:42Z Command-line arg: port="9654"
2025-03-27T13:54:42Z Using at most 125 automatic connections (65535 file descriptors available)
2025-03-27T13:54:42Z
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016764944)
In 9d3be36f758095167ddd762400fda1a3ea76e8f9: This patch was rebased, but I compared a Guix build for `arm64-apple-darwin` on aarch64 & x86-64 with the patch removed, and the builds matched. Did you see otherwise?
https://github.com/fanquake/bitcoin/tree/rem_no_opt (aarch64 & x86_64):
```bash
21e666fafe96478bdcd4a4ea38a80f94aa82e22476b85933d807802bde2640bc guix-build-2faea9a49831/output/arm64-apple-darwin/SHA256SUMS.part
fc37f23342083a8849f64f508448f273258a5fd124989e6501bfef53c016e568 gui
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016764944)
In 9d3be36f758095167ddd762400fda1a3ea76e8f9: This patch was rebased, but I compared a Guix build for `arm64-apple-darwin` on aarch64 & x86-64 with the patch removed, and the builds matched. Did you see otherwise?
https://github.com/fanquake/bitcoin/tree/rem_no_opt (aarch64 & x86_64):
```bash
21e666fafe96478bdcd4a4ea38a80f94aa82e22476b85933d807802bde2640bc guix-build-2faea9a49831/output/arm64-apple-darwin/SHA256SUMS.part
fc37f23342083a8849f64f508448f273258a5fd124989e6501bfef53c016e568 gui
...
💬 laanwj commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758206627)
> 2025-03-27T13:54:46Z [net] portmap: Could not determine IPv4 default gateway
Ok, that's the problem.
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758206627)
> 2025-03-27T13:54:46Z [net] portmap: Could not determine IPv4 default gateway
Ok, that's the problem.
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016781416)
In In 9d3be36f758095167ddd762400fda1a3ea76e8f9:
> symbol, which the linker cannot resolve during cross-compilation.
Which linker doesn't this work with? I dropped this patch, and cross-compiled for macOS on Ubuntu, with lld `18.1.3`, and it seemed to work fine? It'd be good if the patch could include the details.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016781416)
In In 9d3be36f758095167ddd762400fda1a3ea76e8f9:
> symbol, which the linker cannot resolve during cross-compilation.
Which linker doesn't this work with? I dropped this patch, and cross-compiled for macOS on Ubuntu, with lld `18.1.3`, and it seemed to work fine? It'd be good if the patch could include the details.
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016756786)
the `blank=True` flag is not needed when you disable private keys.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016756786)
the `blank=True` flag is not needed when you disable private keys.
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016767447)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
The first check is redundant —`create_legacy_wallet()` already performs it. The other two checks are unnecessary. You want to verify migration here, not whether `createwallet()` works as expected, that's something different.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016767447)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
The first check is redundant —`create_legacy_wallet()` already performs it. The other two checks are unnecessary. You want to verify migration here, not whether `createwallet()` works as expected, that's something different.
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016780253)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
This is actually asserting the wrong behavior. The migrated wallet should have private keys disabled, just like the legacy wallet.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016780253)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
This is actually asserting the wrong behavior. The migrated wallet should have private keys disabled, just like the legacy wallet.
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016751555)
In 9962614b9060:
I changed my mind here. The blank flag check seems redundant and could even be harmful if it isn't unset after key import or creation for some reason. This could just be a:
```c++
success = empty_wallet;
```
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016751555)
In 9962614b9060:
I changed my mind here. The blank flag check seems redundant and could even be harmful if it isn't unset after key import or creation for some reason. This could just be a:
```c++
success = empty_wallet;
```
💬 furszy commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016770083)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
this check is redundant, `migrate_and_get_rpc()` already checks it.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016770083)
In https://github.com/bitcoin/bitcoin/commit/9962614b9060e9713042de91dc04ff5ea6604ae1:
this check is redundant, `migrate_and_get_rpc()` already checks it.
🤔 fjahr reviewed a pull request: "test: Add encodable PUSHDATA1 examples to feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2721891184)
Code review ACK 3dd29b12e1931147145251944d4a1b733be841f9
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2721891184)
Code review ACK 3dd29b12e1931147145251944d4a1b733be841f9
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016791355)
In 9d3be36f758095167ddd762400fda1a3ea76e8f9: This patch was rebased, but testing a Windows build with LTO and this branch, it doesn't seem to compile. So it seems like we should either fixup the patch to make builds under LTO work, or drop the patch and re-add support for LTO later?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2016791355)
In 9d3be36f758095167ddd762400fda1a3ea76e8f9: This patch was rebased, but testing a Windows build with LTO and this branch, it doesn't seem to compile. So it seems like we should either fixup the patch to make builds under LTO work, or drop the patch and re-add support for LTO later?
🤔 pablomartin4btc reviewed a pull request: "wallet: remove redundant `Assert` call when block is disconnected"
(https://github.com/bitcoin/bitcoin/pull/32153#pullrequestreview-2721901787)
cr ACK
(https://github.com/bitcoin/bitcoin/pull/32153#pullrequestreview-2721901787)
cr ACK
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016804703)
Ok, I'll check, I've tested already that when you import an address the flag gets unset.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016804703)
Ok, I'll check, I've tested already that when you import an address the flag gets unset.
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016809896)
But wanted to make it explicit that as the wallet is empty, not imported address, the flag will remain the same after migration.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016809896)
But wanted to make it explicit that as the wallet is empty, not imported address, the flag will remain the same after migration.
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016810958)
Ok, I'll remove it.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2016810958)
Ok, I'll remove it.