💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430148600)
> No, `m_offsets` is not uninitialized
The array is initialized, but the items are not, and afaik `int64_t` doesn't have a default constructor?
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430148600)
> No, `m_offsets` is not uninitialized
The array is initialized, but the items are not, and afaik `int64_t` doesn't have a default constructor?
💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430149707)
Care to elaborate why? Since this is newly introduced code, seems sensible to do it right away?
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430149707)
Care to elaborate why? Since this is newly introduced code, seems sensible to do it right away?
🤔 ismaelsadeeq reviewed a pull request: "fuzz: make FuzzedDataProvider usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1786902962)
>Good point, I guess since the spec is unspecified they have the right to change it but I would still be surprised if they do. I'm not sure if there is a trivial way to check if they ever have.
The order of evaluation issue is a characteristic of the C++ language standard. While the LLVM compiler might have a consistent order of evaluation, but generally compiler behavior could change, maybe we should not use the current LLVM behavior as validation for this but rather refer to the overall lan
...
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1786902962)
>Good point, I guess since the spec is unspecified they have the right to change it but I would still be surprised if they do. I'm not sure if there is a trivial way to check if they ever have.
The order of evaluation issue is a characteristic of the C++ language standard. While the LLVM compiler might have a consistent order of evaluation, but generally compiler behavior could change, maybe we should not use the current LLVM behavior as validation for this but rather refer to the overall lan
...
💬 ismaelsadeeq commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1430148489)
Nit: use descriptive variable names for more clarity instead of single letters
<details>
<summary>see diff</summary>
```diff
diff --git a/src/test/fuzz/crypto.cpp b/src/test/fuzz/crypto.cpp
index aa478277e35..2fa2b109e55 100644
--- a/src/test/fuzz/crypto.cpp
+++ b/src/test/fuzz/crypto.cpp
@@ -23,8 +23,8 @@ FUZZ_TARGET(crypto)
std::vector<uint8_t> data = ConsumeRandomLengthByteVector(fuzzed_data_provider);
if (data.empty()) {
auto new_size = fuzzed_data_prov
...
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1430148489)
Nit: use descriptive variable names for more clarity instead of single letters
<details>
<summary>see diff</summary>
```diff
diff --git a/src/test/fuzz/crypto.cpp b/src/test/fuzz/crypto.cpp
index aa478277e35..2fa2b109e55 100644
--- a/src/test/fuzz/crypto.cpp
+++ b/src/test/fuzz/crypto.cpp
@@ -23,8 +23,8 @@ FUZZ_TARGET(crypto)
std::vector<uint8_t> data = ConsumeRandomLengthByteVector(fuzzed_data_provider);
if (data.empty()) {
auto new_size = fuzzed_data_prov
...
💬 dergoegge commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430151965)
https://godbolt.org/z/5PKET7E39
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430151965)
https://godbolt.org/z/5PKET7E39
💬 ismaelsadeeq commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1430154424)
This is okay since the standard is left-to-right evaluation order for initializer-clauses within an initializer-list?
(https://github.com/bitcoin/bitcoin/pull/29043#discussion_r1430154424)
This is okay since the standard is left-to-right evaluation order for initializer-clauses within an initializer-list?
💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430166505)
Ah cool, didn't know, thanks. Still, having default (0-)values in sorted_copy seems like it would lead to an incorrect median calculation?
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1430166505)
Ah cool, didn't know, thanks. Still, having default (0-)values in sorted_copy seems like it would lead to an incorrect median calculation?
🚀 fanquake merged a pull request: "fuzz: Improve fuzzing stability for minisketch harness"
(https://github.com/bitcoin/bitcoin/pull/29064)
(https://github.com/bitcoin/bitcoin/pull/29064)
💬 sipa commented on issue "Performance decrease after tapscript miniscript":
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860568169)
That sounds very plausible, @darosior.
If so, I think there is an approach that doesn't lose optimality: keep track of the optimal satisfaction size, and once reached, don't bother evaluation further keys?
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860568169)
That sounds very plausible, @darosior.
If so, I think there is an approach that doesn't lose optimality: keep track of the optimal satisfaction size, and once reached, don't bother evaluation further keys?
📝 maflcko opened a pull request: "refactor: Replace ALWAYS_FALSE with false"
(https://github.com/bitcoin/bitcoin/pull/29108)
Now that `static_assert(false)` when evaluated in a non-instantiated template is treated as a no-op, use it over the workaround, and remove the workaround.
(https://github.com/bitcoin/bitcoin/pull/29108)
Now that `static_assert(false)` when evaluated in a non-instantiated template is treated as a no-op, use it over the workaround, and remove the workaround.
💬 darosior commented on issue "Performance decrease after tapscript miniscript":
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860578924)
Neat. This would give us the same performance in the large majority of cases while still being optimal in the rare non-SIGHASH_DEFAULT cases. In any case this would fix the slowdown you noticed @eriknylund.
(https://github.com/bitcoin/bitcoin/issues/29098#issuecomment-1860578924)
Neat. This would give us the same performance in the large majority of cases while still being optimal in the rare non-SIGHASH_DEFAULT cases. In any case this would fix the slowdown you noticed @eriknylund.
✅ maflcko closed a pull request: "refactor: Replace ALWAYS_FALSE with false"
(https://github.com/bitcoin/bitcoin/pull/29108)
(https://github.com/bitcoin/bitcoin/pull/29108)
💬 maflcko commented on pull request "refactor: Replace ALWAYS_FALSE with false":
(https://github.com/bitcoin/bitcoin/pull/29108#issuecomment-1860587423)
Oh nvm, this is C++26
(https://github.com/bitcoin/bitcoin/pull/29108#issuecomment-1860587423)
Oh nvm, this is C++26
⚠️ c0deright opened an issue: "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect"
(https://github.com/bitcoin/bitcoin/issues/29109)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When starting bitcoind with an old wallet (created July 2019, probably `v0.17.x` or `v0.18.x`) the log records 2 errors and one warning that are not self-explanatory.
Might be related to #19051
### Expected behaviour
No warning/errors are shown or the errors/warnings are more detailed as to what's going on.
### Steps to reproduce
Run v26.0 with a very old `wallet.dat` file. I wasn't
...
(https://github.com/bitcoin/bitcoin/issues/29109)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
When starting bitcoind with an old wallet (created July 2019, probably `v0.17.x` or `v0.18.x`) the log records 2 errors and one warning that are not self-explanatory.
Might be related to #19051
### Expected behaviour
No warning/errors are shown or the errors/warnings are more detailed as to what's going on.
### Steps to reproduce
Run v26.0 with a very old `wallet.dat` file. I wasn't
...
💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1430198623)
I was just thinking on ways to simplify this. `CheckForStaleTipAndEvictPeers()` is executed every 45 seconds. I think it is ok if for a while (~ 10 mins) we think we are close to the tip but we are not in practice. Given that we realize this in a few minutes and don't get stuck with the wrong assumption forever.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1430198623)
I was just thinking on ways to simplify this. `CheckForStaleTipAndEvictPeers()` is executed every 45 seconds. I think it is ok if for a while (~ 10 mins) we think we are close to the tip but we are not in practice. Given that we realize this in a few minutes and don't get stuck with the wrong assumption forever.
👍 instagibbs approved a pull request: "wallet, mempool: propagete `checkChainLimits` error message to wallet"
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1787005073)
LGTM
(https://github.com/bitcoin/bitcoin/pull/28863#pullrequestreview-1787005073)
LGTM
💬 instagibbs commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430210897)
this would be good
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430210897)
this would be good
💬 ChrisMartl commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860650558)
Concept ACK
Database is a very important resource for the "Bitcoin"-System and its usage must be reserved exclusively for Sats endogenous monetary usage.
Since the Bitcoin's systemic flaw (https://github.com/bitcoin/bitcoin/issues/29089#issue-2043281653) is still unaddressed / unsolved, efforts to preserve the capability of already allocated storage devices resources in the network shall be considered and encouraged.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1860650558)
Concept ACK
Database is a very important resource for the "Bitcoin"-System and its usage must be reserved exclusively for Sats endogenous monetary usage.
Since the Bitcoin's systemic flaw (https://github.com/bitcoin/bitcoin/issues/29089#issue-2043281653) is still unaddressed / unsolved, efforts to preserve the capability of already allocated storage devices resources in the network shall be considered and encouraged.
💬 ismaelsadeeq commented on pull request "wallet, mempool: propagete `checkChainLimits` error message to wallet":
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430217862)
Thanks Would create a tiny follow-up to fix this, so as not to invalidate ACK's.
(https://github.com/bitcoin/bitcoin/pull/28863#discussion_r1430217862)
Thanks Would create a tiny follow-up to fix this, so as not to invalidate ACK's.
💬 brunoerg commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1860680045)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1860680045)
Concept ACK