💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099637)
This is fine, but I think for documentation purposes, the static globals are initialized in `initialize_setup` in the fuzz tests
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193099637)
This is fine, but I think for documentation purposes, the static globals are initialized in `initialize_setup` in the fuzz tests
💬 MarcoFalke commented on pull request "fuzz: wallet, add target for `fees`":
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193100074)
nit for less code (feel free to ignore):
```suggestion
FeeCalculation fee_calculation;
FeeCalculation* maybe_fee_calculation{ fuzzed_data_provider.ConsumeBool()?nullptr:&fee_calculation};
(void)GetMinimumFeeRate(wallet, coin_control, maybe_fee_calculation);
(void)GetMinimumFee(wallet, tx_bytes, coin_control, maybe_fee_calculation);
```
(https://github.com/bitcoin/bitcoin/pull/27647#discussion_r1193100074)
nit for less code (feel free to ignore):
```suggestion
FeeCalculation fee_calculation;
FeeCalculation* maybe_fee_calculation{ fuzzed_data_provider.ConsumeBool()?nullptr:&fee_calculation};
(void)GetMinimumFeeRate(wallet, coin_control, maybe_fee_calculation);
(void)GetMinimumFee(wallet, tx_bytes, coin_control, maybe_fee_calculation);
```
🚀 fanquake merged a pull request: "build: Fix shared lib linking for darwin with lld"
(https://github.com/bitcoin/bitcoin/pull/27628)
(https://github.com/bitcoin/bitcoin/pull/27628)
🚀 fanquake merged a pull request: "depends: no-longer nuke libc++abi.so* in native_clang package"
(https://github.com/bitcoin/bitcoin/pull/27493)
(https://github.com/bitcoin/bitcoin/pull/27493)
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1546869753)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1546869753)
Concept ACK.
🤔 hebasto reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1425535676)
Approach ACK 63e915af5a437d36abdd79a1b90fed2395086589.
> The approach implemented here introduces some indirection, which makes the code a bit harder to read.
As for me, it does not look like a problem at all. Good function naming works well :D
In commit 73556797ab9c44038994954b945db996642656d2:
```diff
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4659,7 +4659,7 @@ void Chainstate::LoadExternalBlockFile(
}
}
- Notif
...
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1425535676)
Approach ACK 63e915af5a437d36abdd79a1b90fed2395086589.
> The approach implemented here introduces some indirection, which makes the code a bit harder to read.
As for me, it does not look like a problem at all. Good function naming works well :D
In commit 73556797ab9c44038994954b945db996642656d2:
```diff
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4659,7 +4659,7 @@ void Chainstate::LoadExternalBlockFile(
}
}
- Notif
...
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193133568)
Shouldn't this change be moved to the first commit?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193133568)
Shouldn't this change be moved to the first commit?
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193134480)
We also don't set other params, like `adjusted_time_callback` in the dummy opts, since they are not required to read the args into the struct. The commit where this is changed, adds it because the `BlockManager::Options` requires a reference to the notifications starting at that commit.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193134480)
We also don't set other params, like `adjusted_time_callback` in the dummy opts, since they are not required to read the args into the struct. The commit where this is changed, adds it because the `BlockManager::Options` requires a reference to the notifications starting at that commit.
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193141876)
FWIW, gcc emits a warning here.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193141876)
FWIW, gcc emits a warning here.
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193146924)
Thank you for following up, will patch.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193146924)
Thank you for following up, will patch.
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1546903132)
Re https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1425535676
> In commit [7355679](https://github.com/bitcoin/bitcoin/commit/73556797ab9c44038994954b945db996642656d2):
>
> ```diff
> --- a/src/validation.cpp
> +++ b/src/validation.cpp
> @@ -4659,7 +4659,7 @@ void Chainstate::LoadExternalBlockFile(
> }
> }
>
> - NotifyHeaderTip(*this);
> + ::NotifyHeaderTip(*this);
>
> if (!blo
...
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1546903132)
Re https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1425535676
> In commit [7355679](https://github.com/bitcoin/bitcoin/commit/73556797ab9c44038994954b945db996642656d2):
>
> ```diff
> --- a/src/validation.cpp
> +++ b/src/validation.cpp
> @@ -4659,7 +4659,7 @@ void Chainstate::LoadExternalBlockFile(
> }
> }
>
> - NotifyHeaderTip(*this);
> + ::NotifyHeaderTip(*this);
>
> if (!blo
...
📝 theStack opened a pull request: "test: add unit test coverage for Python ECDSA implementation"
(https://github.com/bitcoin/bitcoin/pull/27653)
This PR adds missing unit test coverage for the Python ECDSA implementation, which should be useful for detecting potential problems early whenever changes in framework's Python implementation of secp256k1 are made (e.g. #26222). Note that right now we don't call `ECPubKey.verify_ecdsa` anywhere in our tests, so we wouldn't notice if it is broken at some point.
To keep it simple, the already existing unit test for Schnorr signatures is extended to also check ECDSA signatures. For that purpose
...
(https://github.com/bitcoin/bitcoin/pull/27653)
This PR adds missing unit test coverage for the Python ECDSA implementation, which should be useful for detecting potential problems early whenever changes in framework's Python implementation of secp256k1 are made (e.g. #26222). Note that right now we don't call `ECPubKey.verify_ecdsa` anywhere in our tests, so we wouldn't notice if it is broken at some point.
To keep it simple, the already existing unit test for Schnorr signatures is extended to also check ECDSA signatures. For that purpose
...
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193161217)
Should these methods be non-`const`?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193161217)
Should these methods be non-`const`?
💬 hebasto commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193161280)
nit: EOL?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193161280)
nit: EOL?
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1546917869)
I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in `mapTxSpends`.
(https://github.com/bitcoin/bitcoin/pull/27145#issuecomment-1546917869)
I've fixed a bug which resulted in the conflicted tx not being marked as inactive if the conflicting tx was not in `mapTxSpends`.
💬 ishaanam commented on pull request "wallet: when a block is disconnected, update transactions that are no longer conflicted":
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1193162426)
There are some tests for that here: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_resendwallettransactions.py. Or did you mean that a test for that should be added for this PR specifically?
(https://github.com/bitcoin/bitcoin/pull/27145#discussion_r1193162426)
There are some tests for that here: https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_resendwallettransactions.py. Or did you mean that a test for that should be added for this PR specifically?
💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193165783)
Yes, as far as I am aware, `const` virtual methods are bad practice. See: https://github.com/bitcoin/bitcoin/pull/25337#discussion_r916367659.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1193165783)
Yes, as far as I am aware, `const` virtual methods are bad practice. See: https://github.com/bitcoin/bitcoin/pull/25337#discussion_r916367659.
⚠️ bleeee opened an issue: "v0.6.5 on iPad mini 6 Security Center button incorrent"
(https://github.com/bitcoin/bitcoin/issues/27654)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
v0.6.5 on iPad mini 6 Security Center button incorrent
2FA button is actually app password
app password button is wall set pass phrase
all wallet encryption button is biometrics
### Expected behaviour
corrention buttons
### Steps to reproduce
press any button in secrity center
### Relevant log output
_No response_
### How did you obtain Bitcoin Core
Compiled from source
###
...
(https://github.com/bitcoin/bitcoin/issues/27654)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
v0.6.5 on iPad mini 6 Security Center button incorrent
2FA button is actually app password
app password button is wall set pass phrase
all wallet encryption button is biometrics
### Expected behaviour
corrention buttons
### Steps to reproduce
press any button in secrity center
### Relevant log output
_No response_
### How did you obtain Bitcoin Core
Compiled from source
###
...
✅ fanquake closed an issue: "v0.6.5 on iPad mini 6 Security Center button incorrent"
(https://github.com/bitcoin/bitcoin/issues/27654)
(https://github.com/bitcoin/bitcoin/issues/27654)
⚠️ kallerosenbaum opened an issue: "Can't start bitcoin-qt by double-click on linux"
(https://github.com/bitcoin/bitcoin/issues/27655)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When I double-click the bitcoin-qt file executable file in the graphical file manager, Bitcoin Core doesn't start and this message pops up:

I think this issue is the same as described at https://fostips.com/double-click-run-elf-ubuntu/
The Bitcoin Core build
...
(https://github.com/bitcoin/bitcoin/issues/27655)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When I double-click the bitcoin-qt file executable file in the graphical file manager, Bitcoin Core doesn't start and this message pops up:

I think this issue is the same as described at https://fostips.com/double-click-run-elf-ubuntu/
The Bitcoin Core build
...