🚀 achow101 merged a pull request: "doc: correct sqlite & qrencode versions used in depenendencies.md"
(https://github.com/bitcoin/bitcoin/pull/27441)
(https://github.com/bitcoin/bitcoin/pull/27441)
📝 benthecarman opened a pull request: "Allow configuirng target block time for a signet"
(https://github.com/bitcoin/bitcoin/pull/27446)
This pull request introduces the ability to configure the block time of a custom Bitcoin signet network, allowing developers to more easily simulate various network scenarios and test their applications in a controlled environment. The change helps to improve the flexibility of signet, making it more useful for diverse testing purposes. For example, I am trying to setup a signet with a 30 second block time and this caused a bunch of difficulty adjustments to happen making the network inconsisten
...
(https://github.com/bitcoin/bitcoin/pull/27446)
This pull request introduces the ability to configure the block time of a custom Bitcoin signet network, allowing developers to more easily simulate various network scenarios and test their applications in a controlled environment. The change helps to improve the flexibility of signet, making it more useful for diverse testing purposes. For example, I am trying to setup a signet with a 30 second block time and this caused a bunch of difficulty adjustments to happen making the network inconsisten
...
💬 ajtowns commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1502605806)
Configurable consensus parameters seems like a great way to make the network inconsistent in general (in particular, if you get the parameter wrong, you'll end up marking valid blocks invalid, with various consequences).
What's the actual problem you're trying to solve here? Would it work to have the signet miner only generate blocks when the mempool has entries, and sit idle when it's empty, for instance? If you're trying to generate many blocks, then regtest is already better as it doesn't
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1502605806)
Configurable consensus parameters seems like a great way to make the network inconsistent in general (in particular, if you get the parameter wrong, you'll end up marking valid blocks invalid, with various consequences).
What's the actual problem you're trying to solve here? Would it work to have the signet miner only generate blocks when the mempool has entries, and sit idle when it's empty, for instance? If you're trying to generate many blocks, then regtest is already better as it doesn't
...
✅ kallewoof closed a pull request: "rpc/doc: describe using combo(privkey) to get checksum and then list …"
(https://github.com/bitcoin/bitcoin/pull/24161)
(https://github.com/bitcoin/bitcoin/pull/24161)
✅ kallewoof closed a pull request: "rpc: add require_checksum flag to deriveaddresses"
(https://github.com/bitcoin/bitcoin/pull/24162)
(https://github.com/bitcoin/bitcoin/pull/24162)
💬 ajtowns commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1162303007)
Setting it to `N` will prevent warnings that would otherwise be triggered by version bits being set in blocks up to `N+2015`; so you pick a value where a reorg back to `N+2015` or lower is sufficiently unlikely (for similar values of "sufficiently unlikely" as we'd use to bury successfully activated soft forks) and that `getblockchaininfo` from above that height doesn't already include an `Unknown new rules activated` warning. (If it does, it's obviously important to verify that the new rules th
...
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1162303007)
Setting it to `N` will prevent warnings that would otherwise be triggered by version bits being set in blocks up to `N+2015`; so you pick a value where a reorg back to `N+2015` or lower is sufficiently unlikely (for similar values of "sufficiently unlikely" as we'd use to bury successfully activated soft forks) and that `getblockchaininfo` from above that height doesn't already include an `Unknown new rules activated` warning. (If it does, it's obviously important to verify that the new rules th
...
💬 ajtowns commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1162304494)
(Maybe it would be better to do `std::max(MinBIP9WarningHeight, nMinerConfirmationWindow) - nMinerConfirmationWindow` in `WarningBitsConditionChecker::BeginTime()` ; then you're just ignoring version bits prior to `MinBIP9WarningHeight`)
(https://github.com/bitcoin/bitcoin/pull/27427#discussion_r1162304494)
(Maybe it would be better to do `std::max(MinBIP9WarningHeight, nMinerConfirmationWindow) - nMinerConfirmationWindow` in `WarningBitsConditionChecker::BeginTime()` ; then you're just ignoring version bits prior to `MinBIP9WarningHeight`)
💬 1440000bytes commented on pull request "Allow configuirng target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1162338423)
```suggestion
argsman.AddArg("-signetblocktime", "Difficulty adjustment will target a block time of the given amount in seconds (only for signet networks; defaults to 10 minutes)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS);
```
(https://github.com/bitcoin/bitcoin/pull/27446#discussion_r1162338423)
```suggestion
argsman.AddArg("-signetblocktime", "Difficulty adjustment will target a block time of the given amount in seconds (only for signet networks; defaults to 10 minutes)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS);
```
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1162375326)
Yes this is nice to have. Since the key format could never change due to backward compatibility requirements, this is not an issue. You can resolve this comment.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1162375326)
Yes this is nice to have. Since the key format could never change due to backward compatibility requirements, this is not an issue. You can resolve this comment.
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1162378013)
I guess that's ok, but `AddHDKey` already has an early return for existing keys.
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1162378013)
I guess that's ok, but `AddHDKey` already has an early return for existing keys.
👍 1440000bytes approved a pull request: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1378615445)
utACK https://github.com/bitcoin/bitcoin/pull/27279/commits/7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
(https://github.com/bitcoin/bitcoin/pull/27279#pullrequestreview-1378615445)
utACK https://github.com/bitcoin/bitcoin/pull/27279/commits/7ccdd741fe1544c13b2a9b7baa5c5727e84d6e55
💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502850609)
Not sure. Requiring clang-16, and valgrind to be compiled from source, along with removing support for gcc seems a bit aggressive.
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502850609)
Not sure. Requiring clang-16, and valgrind to be compiled from source, along with removing support for gcc seems a bit aggressive.
💬 fanquake commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502851953)
What do you mean by "removing support for GCC"?
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502851953)
What do you mean by "removing support for GCC"?
💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1502851999)
I've been trying to reproduce this by re-running the CI on master https://github.com/bitcoin/bitcoin/commit/49b87bfe7e2799d25ce709123ecafa872b36e87a: https://cirrus-ci.com/task/4749392670883840
So far all runs passed..
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1502851999)
I've been trying to reproduce this by re-running the CI on master https://github.com/bitcoin/bitcoin/commit/49b87bfe7e2799d25ce709123ecafa872b36e87a: https://cirrus-ci.com/task/4749392670883840
So far all runs passed..
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1502863992)
Thanks for the helpful comments @willcl-ark, @martinus and @kouloumos! I plan to address these and update this PR with your suggestions. Currently, I'm prioritizing other projects during the feature freeze, so it might take a few more weeks.
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1502863992)
Thanks for the helpful comments @willcl-ark, @martinus and @kouloumos! I plan to address these and update this PR with your suggestions. Currently, I'm prioritizing other projects during the feature freeze, so it might take a few more weeks.
💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502875175)
You removed the suppression for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65434 without explanation? (Or with the explanation that it is an unused GUI supppression?)
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502875175)
You removed the suppression for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65434 without explanation? (Or with the explanation that it is an unused GUI supppression?)
💬 fanquake commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502888918)
> You removed the suppression for
I removed that suppression as I can't seem to recreate the issue. I assume it's been "fixed" due to using the newer libstdc++ versions shipped with the newer Ubuntu. Note that this was the case with clang-16 & the ubuntu shipped valgrind 3.19 as well.
The GUI suppression was removed because it's unused, and any versions would be out of date when changing Ubuntu versions.
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502888918)
> You removed the suppression for
I removed that suppression as I can't seem to recreate the issue. I assume it's been "fixed" due to using the newer libstdc++ versions shipped with the newer Ubuntu. Note that this was the case with clang-16 & the ubuntu shipped valgrind 3.19 as well.
The GUI suppression was removed because it's unused, and any versions would be out of date when changing Ubuntu versions.
💬 0xB10C commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1502895511)
Concept ACK and thank you for looking into this!
I would include this in https://github.com/bitcoin/bitcoin/pull/26593 (e.g. with https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768) too. If https://github.com/bitcoin/bitcoin/issues/26916 is blocking something (e.g. switching to a newer clang) I think merging this PR here first should be preferred as https://github.com/bitcoin/bitcoin/pull/26593 is a bigger change that might take a bit longer.
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1502895511)
Concept ACK and thank you for looking into this!
I would include this in https://github.com/bitcoin/bitcoin/pull/26593 (e.g. with https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768) too. If https://github.com/bitcoin/bitcoin/issues/26916 is blocking something (e.g. switching to a newer clang) I think merging this PR here first should be preferred as https://github.com/bitcoin/bitcoin/pull/26593 is a bigger change that might take a bit longer.
💬 MarcoFalke commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1502901268)
Maybe it reproduces faster with `rr`?
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-1502901268)
Maybe it reproduces faster with `rr`?
💬 MarcoFalke commented on pull request "test: build Valgrind (3.20) from source & use Clang 16":
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502941860)
In any case, it would be good to provide a reason for the bump. Using non-LTS devel OS releases in CI should be an exception reserved for bugfixes or important features, not the norm.
No objection if you want to use bookworm, but for lunar there should be a rationale.
(https://github.com/bitcoin/bitcoin/pull/27444#issuecomment-1502941860)
In any case, it would be good to provide a reason for the bump. Using non-LTS devel OS releases in CI should be an exception reserved for bugfixes or important features, not the norm.
No objection if you want to use bookworm, but for lunar there should be a rationale.