💬 fanquake commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2442781213)
> I'd like to keep it open as I explore less nuclear solutions. Pushed an initial attempt to here and my personal master: hodlinator/bitcoin@84cd647...f69a238
In my opinion your (already merged) change to drop the offending code from the RNG is fine, and isn't really something we need to investigate further / work on reintroducing.
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2442781213)
> I'd like to keep it open as I explore less nuclear solutions. Pushed an initial attempt to here and my personal master: hodlinator/bitcoin@84cd647...f69a238
In my opinion your (already merged) change to drop the offending code from the RNG is fine, and isn't really something we need to investigate further / work on reintroducing.
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1819836232)
I'm afraid `FuzzedDataProvider::remaining_bytes_` becomes zero after this line in the current version, meaning `provider.ConsumeIntegralInRange<int>(0, decoded.size() - 1)` below will always return the minimum value if I'm reading the `ConsumeIntegralInRange`-implementation correctly.
```suggestion
const std::string random_string{provider.ConsumeRandomLengthString(101)};
```
---
Could maybe (in another PR) add something like `bool FuzzedDataProvider::consume_remaining_called_{false}
...
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1819836232)
I'm afraid `FuzzedDataProvider::remaining_bytes_` becomes zero after this line in the current version, meaning `provider.ConsumeIntegralInRange<int>(0, decoded.size() - 1)` below will always return the minimum value if I'm reading the `ConsumeIntegralInRange`-implementation correctly.
```suggestion
const std::string random_string{provider.ConsumeRandomLengthString(101)};
```
---
Could maybe (in another PR) add something like `bool FuzzedDataProvider::consume_remaining_called_{false}
...
💬 cobratbq commented on issue "Docs: "External signer" documentation is outdated. (plz close if unwanted request)":
(https://github.com/bitcoin/bitcoin/issues/31005#issuecomment-2442820613)
When using `--stdin`, there is `1` command, and `2` variations of input (so far):
- `signtx` (space-separated)
- _Bitcoin Core v27.x_: `"<base64-encoded (binary) PSBT>"` (with double-quotes literally present and tag between `<` and `>` replaced with its representative content)
- _Bitcoin Core v28.x_: `<base64-encoded (binary) PSBT>` (without double-quotes, same PSBT representation)
(https://github.com/bitcoin/bitcoin/issues/31005#issuecomment-2442820613)
When using `--stdin`, there is `1` command, and `2` variations of input (so far):
- `signtx` (space-separated)
- _Bitcoin Core v27.x_: `"<base64-encoded (binary) PSBT>"` (with double-quotes literally present and tag between `<` and `>` replaced with its representative content)
- _Bitcoin Core v28.x_: `<base64-encoded (binary) PSBT>` (without double-quotes, same PSBT representation)
📝 ryanofsky opened a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174)
This PR is an alternative to #31149 that **only** adds compile-time checking for literal format strings and does not make other changes in the codebase.
(https://github.com/bitcoin/bitcoin/pull/31174)
This PR is an alternative to #31149 that **only** adds compile-time checking for literal format strings and does not make other changes in the codebase.
💬 ryanofsky commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2442899726)
Sorry for causing unnecessary churn with my earlier comments. After reading Marco's comment in https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801085975 I finally understand what this PR is really trying to do, and why all the changes are being made here. Before reading that comment I thought this PR was trying not just to add compile-time checking for tinyformat format strings, but also to force calls that don't use compile-time checking to use different formatting functions like `fo
...
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2442899726)
Sorry for causing unnecessary churn with my earlier comments. After reading Marco's comment in https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801085975 I finally understand what this PR is really trying to do, and why all the changes are being made here. Before reading that comment I thought this PR was trying not just to add compile-time checking for tinyformat format strings, but also to force calls that don't use compile-time checking to use different formatting functions like `fo
...
💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1820218726)
> Could maybe (in another PR)
In theory it could also be caught at compile time with https://clang.llvm.org/docs/AttributeReference.html#consumed-annotation-checking
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1820218726)
> Could maybe (in another PR)
In theory it could also be caught at compile time with https://clang.llvm.org/docs/AttributeReference.html#consumed-annotation-checking
👍 rkrux approved a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2400959381)
tACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5
Successful make and all functional tests. This is my preference as well to use the `getorphantxs` RPC for asserting over searching in the debug logs.
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2400959381)
tACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5
Successful make and all functional tests. This is my preference as well to use the `getorphantxs` RPC for asserting over searching in the debug logs.
💬 laanwj commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2443481677)
> isn't really something we need to investigate further / work on reintroducing.
Agree, it's fine, it's good to get rid of the perfmon query.
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2443481677)
> isn't really something we need to investigate further / work on reintroducing.
Agree, it's fine, it's good to get rid of the perfmon query.
💬 maflcko commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1820289265)
nit: For new code it would be good to use the alias `chainman().GetMutex()` member instead, when `chainman()` members are accessed.
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1820289265)
nit: For new code it would be good to use the alias `chainman().GetMutex()` member instead, when `chainman()` members are accessed.
💬 jarolrod commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2443594716)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2443594716)
Concept ACK
💬 jarolrod commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2443600588)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2443600588)
Concept ACK
💬 jarolrod commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2443601472)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2443601472)
Concept ACK
🤔 jarolrod reviewed a pull request: "Decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2401161662)
strong concept ack
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2401161662)
strong concept ack
💬 jarolrod commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443649838)
> but merely to give a feeling and to avoid a regression where an algorithm is swapped to go from O(n) to O(n^2). So in a sense, they are a bit of a unit test.
This is a good reason to keep.
> I'm arguing for scoping our tests to the latter (and other critical things e.g. mining) so that we don't get distracted.
And this is an important statement.
We don't necessarily need to throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues
...
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443649838)
> but merely to give a feeling and to avoid a regression where an algorithm is swapped to go from O(n) to O(n^2). So in a sense, they are a bit of a unit test.
This is a good reason to keep.
> I'm arguing for scoping our tests to the latter (and other critical things e.g. mining) so that we don't get distracted.
And this is an important statement.
We don't necessarily need to throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues
...
💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2443665126)
> Looks like that fixed it. However, it would be good to understand _why_ it fixed it. Looking at the diff that triggered it, I fail to see how ([#31000 (comment)](https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680))
Running on another branch with `-testdatadir=foo` I get files like:
`foo/test_common bitcoin/validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch/datadir/regtest/chainstate_snapshot_INVALID/MANIFEST-000004`
Combined with `C
...
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2443665126)
> Looks like that fixed it. However, it would be good to understand _why_ it fixed it. Looking at the diff that triggered it, I fail to see how ([#31000 (comment)](https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680))
Running on another branch with `-testdatadir=foo` I get files like:
`foo/test_common bitcoin/validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch/datadir/regtest/chainstate_snapshot_INVALID/MANIFEST-000004`
Combined with `C
...
💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2443678171)
> Running on another branch with `-testdatadir=foo`
The Windows CI doesn't set the flag and the push/diff I linked to didn't change the non `-testdatadir` code path, but the previous commit passed and the next one failed. Just asking to understand what is going on with the CI.
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2443678171)
> Running on another branch with `-testdatadir=foo`
The Windows CI doesn't set the flag and the push/diff I linked to didn't change the non `-testdatadir` code path, but the previous commit passed and the next one failed. Just asking to understand what is going on with the CI.
💬 hodlinator commented on pull request "util: Remove RandAddSeedPerfmon":
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2443686717)
> The high quality OS entropy source on windows is `CryptGenRandom`, and we use that.
[A Microsoft article](https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom) from 2021 on `CryptGenRandom` states:
> **Important** This API is deprecated. New and existing software should start using [Cryptography Next Generation APIs.](https://learn.microsoft.com/en-us/windows/desktop/SecCNG/cng-portal) Microsoft may remove this API in future releases.
So maybe we coul
...
(https://github.com/bitcoin/bitcoin/pull/31124#issuecomment-2443686717)
> The high quality OS entropy source on windows is `CryptGenRandom`, and we use that.
[A Microsoft article](https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom) from 2021 on `CryptGenRandom` states:
> **Important** This API is deprecated. New and existing software should start using [Cryptography Next Generation APIs.](https://learn.microsoft.com/en-us/windows/desktop/SecCNG/cng-portal) Microsoft may remove this API in future releases.
So maybe we coul
...
💬 laanwj commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820458290)
Maybe add something like "if the IDE has an option for this" to make it slightly more clear, out of context, that it's an IDE option, and not something to set with say, `debugedit`.
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1820458290)
Maybe add something like "if the IDE has an option for this" to make it slightly more clear, out of context, that it's an IDE option, and not something to set with say, `debugedit`.
💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)
Interesting. So, for me, the first commit (with a new test) takes forever to execute.
More specifically, the execution hangs on `best_peers.resize(targets_size);`, with `targets_size` being a huge number.
I guess your system handles this gracefully instead?
A clear failure could be triggered by e.g. `Assume(targets_size <= m_states.size());`
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1820474026)
Interesting. So, for me, the first commit (with a new test) takes forever to execute.
More specifically, the execution hangs on `best_peers.resize(targets_size);`, with `targets_size` being a huge number.
I guess your system handles this gracefully instead?
A clear failure could be triggered by e.g. `Assume(targets_size <= m_states.size());`
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443766239)
> throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues/17501)
The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2443766239)
> throw away work that has previously been a useful metric in issues (https://github.com/bitcoin/bitcoin/issues/17501)
The benchmarks weren't mentioned in the issue you linked or the PR that fixed the issue?