🤔 furszy reviewed a pull request: "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)"
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-3378984656)
Tested ACK 0465574c127907df9b764055a585e8281bae8d1d.
Reverting the fix makes the introduced test fails (after ~60 test runs).
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-3378984656)
Tested ACK 0465574c127907df9b764055a585e8281bae8d1d.
Reverting the fix makes the introduced test fails (after ~60 test runs).
🤔 stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3369762303)
Comments on commits 3 (92f54852) through 6 (f9bf4d9b) - context and chainstate manager. Also includes a couple of follow-ups from previous review comments on logging.
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3369762303)
Comments on commits 3 (92f54852) through 6 (f9bf4d9b) - context and chainstate manager. Also includes a couple of follow-ups from previous review comments on logging.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2457664989)
Could this be simplified to wrap `CChainParams` directly instead? (to match the pattern where handles either wrap types directly or wrap `shared_ptr`?
<details>
<summary>diff</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 10cf741bd1..49749b312a 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -481,7 +481,7 @@ struct btck_ScriptPubkey : Handle<btck_ScriptPubkey, CScript> {};
struct btck_LoggingConnection
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2457664989)
Could this be simplified to wrap `CChainParams` directly instead? (to match the pattern where handles either wrap types directly or wrap `shared_ptr`?
<details>
<summary>diff</summary>
```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index 10cf741bd1..49749b312a 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -481,7 +481,7 @@ struct btck_ScriptPubkey : Handle<btck_ScriptPubkey, CScript> {};
struct btck_LoggingConnection
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2455010438)
```suggestion
/**
* Opaque data structure for holding options for creating a new kernel context.
*
* Once a kernel context has been created from these options, they may be
* destroyed. The options hold the notification callbacks and validation interface callbacks as well as the
* selected chain type until they are passed to the context. If no options are
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2455010438)
```suggestion
/**
* Opaque data structure for holding options for creating a new kernel context.
*
* Once a kernel context has been created from these options, they may be
* destroyed. The options hold the notification callbacks and validation interface callbacks as well as the
* selected chain type until they are passed to the context. If no options are
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459437404)
Lines 365-366 have changes in commit 92f5485. It seems they should have been included in the previous commit - 3204110. Also line 770 in the same file.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459437404)
Lines 365-366 have changes in commit 92f5485. It seems they should have been included in the previous commit - 3204110. Also line 770 in the same file.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459999505)
Can we omit virtual methods here? (also for `ValidationInterface`)
<details>
<summary>diff</summary>
```diff
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
index 98862b8d05..7fb7c2bc6f 100644
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -812,21 +812,19 @@ template <typename T>
class KernelNotifications
{
public:
- virtual ~KernelNotifications() = default;
+ void BlockTipHandler(SynchronizationSt
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459999505)
Can we omit virtual methods here? (also for `ValidationInterface`)
<details>
<summary>diff</summary>
```diff
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
index 98862b8d05..7fb7c2bc6f 100644
--- a/src/kernel/bitcoinkernel_wrapper.h
+++ b/src/kernel/bitcoinkernel_wrapper.h
@@ -812,21 +812,19 @@ template <typename T>
class KernelNotifications
{
public:
- virtual ~KernelNotifications() = default;
+ void BlockTipHandler(SynchronizationSt
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2461486493)
Last line seems unnecessary.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2461486493)
Last line seems unnecessary.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2461385314)
In commit 3270c061 description: it is better to drop "It is only valid if that context remains in memory too" now that context is a `shared_ptr` shared among objects that depend on it (as the above doc correctly indicates).
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2461385314)
In commit 3270c061 description: it is better to drop "It is only valid if that context remains in memory too" now that context is a `shared_ptr` shared among objects that depend on it (as the above doc correctly indicates).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459690268)
Last line seems unnecessary.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459690268)
Last line seems unnecessary.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2461974025)
Replying [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445236632) comment: shouldn't we have the following change to reflect that?
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 5e108f8949..6e4fc188fb 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -285,9 +285,9 @@ void CheckHandle(T object, T distinct_object)
// Copy constructor
T object2(distinct_object);
- BOOST_C
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2461974025)
Replying [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2445236632) comment: shouldn't we have the following change to reflect that?
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index 5e108f8949..6e4fc188fb 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -285,9 +285,9 @@ void CheckHandle(T object, T distinct_object)
// Copy constructor
T object2(distinct_object);
- BOOST_C
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2462078269)
Replying [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2451399250) comment:
Related concern about `btck_logging_set_options()` (which also existed before introducing the function): if a purpose for `cs_main` here is to protect the logging config variables, but logging operations read those same variables under `m_cs` (not `cs_main`), don't we have unsynchronized reads/writes? The config writes happen under `cs_main` while `LogTimestampStr()` and similar functions read `m_lo
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2462078269)
Replying [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2451399250) comment:
Related concern about `btck_logging_set_options()` (which also existed before introducing the function): if a purpose for `cs_main` here is to protect the logging config variables, but logging operations read those same variables under `m_cs` (not `cs_main`), don't we have unsynchronized reads/writes? The config writes happen under `cs_main` while `LogTimestampStr()` and similar functions read `m_lo
...
🤔 AndricoSean reviewed a pull request: "test: Update BIP324 test vectors"
(https://github.com/bitcoin/bitcoin/pull/33688#pullrequestreview-3379176181)
lgtm
(https://github.com/bitcoin/bitcoin/pull/33688#pullrequestreview-3379176181)
lgtm
💬 Sammie05 commented on pull request "test: Use same rpc timeout for authproxy and cli":
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3445282493)
Tested PR #33698
I went through the new changes, built the code, ran the specific test p2p_headers_sync_with_minchainwork.py --timeout-factor=0.2 in both RPC and CLI modes and everything looks good . but why did we decide on half the timeout for CLI instead of using the same value?
(https://github.com/bitcoin/bitcoin/pull/33698#issuecomment-3445282493)
Tested PR #33698
I went through the new changes, built the code, ran the specific test p2p_headers_sync_with_minchainwork.py --timeout-factor=0.2 in both RPC and CLI modes and everything looks good . but why did we decide on half the timeout for CLI instead of using the same value?
📝 yancyribbens opened a pull request: "test: add case where `TOTAL_TRIES` is exceeded yet solution remains"
(https://github.com/bitcoin/bitcoin/pull/33701)
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before e
...
(https://github.com/bitcoin/bitcoin/pull/33701)
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before e
...
💬 yancyribbens commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#issuecomment-3445337171)
closes https://github.com/bitcoin/bitcoin/issues/33419
(https://github.com/bitcoin/bitcoin/pull/33701#issuecomment-3445337171)
closes https://github.com/bitcoin/bitcoin/issues/33419
💬 diegoviola commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3445467168)
> At most, I expect to receive a notification informing the user of that fact, but not actually stealing keyboard focus from the current application.
Not quite right:
> Consolatis │ dviola: I'd expect most compositors to raise and give keyboard focus to a surface requesting activation with a valid token (e.g. the surface the token originated from had keyboard or pointer focus at the time of issuing the token and not too much time has passed). I think some compositors treat "invalid" tokens
...
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-3445467168)
> At most, I expect to receive a notification informing the user of that fact, but not actually stealing keyboard focus from the current application.
Not quite right:
> Consolatis │ dviola: I'd expect most compositors to raise and give keyboard focus to a surface requesting activation with a valid token (e.g. the surface the token originated from had keyboard or pointer focus at the time of issuing the token and not too much time has passed). I think some compositors treat "invalid" tokens
...
💬 ploo6669 commented on issue "SENDTEMPLATE Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/33691#issuecomment-3445975830)
the error is 1 bit video card
CPU has stable bytes
(https://github.com/bitcoin/bitcoin/issues/33691#issuecomment-3445975830)
the error is 1 bit video card
CPU has stable bytes
💬 maflcko commented on pull request "refactor/doc: Add blockman param to GetTransaction doc comment":
(https://github.com/bitcoin/bitcoin/pull/33683#issuecomment-3446088430)
re-lgtm-ut-cr-ACK 1a1f46c2285994908df9c11991c1f363c9733087
(https://github.com/bitcoin/bitcoin/pull/33683#issuecomment-3446088430)
re-lgtm-ut-cr-ACK 1a1f46c2285994908df9c11991c1f363c9733087
👍 rkrux approved a pull request: "rpc: add optional peer_ids param to filter getpeerinfo"
(https://github.com/bitcoin/bitcoin/pull/32741#pullrequestreview-3379820927)
ACK [8471074](https://github.com/bitcoin/bitcoin/pull/32741/commits/8471074fb996692fc56acbc34abe83103e084821)
(https://github.com/bitcoin/bitcoin/pull/32741#pullrequestreview-3379820927)
ACK [8471074](https://github.com/bitcoin/bitcoin/pull/32741/commits/8471074fb996692fc56acbc34abe83103e084821)
👍 maflcko approved a pull request: "qt: add createwallet, createwalletdescriptor, and migratewallet to history filter"
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3379853438)
lgtm, seems fine to add. Left a question
(https://github.com/bitcoin-core/gui/pull/901#pullrequestreview-3379853438)
lgtm, seems fine to add. Left a question