💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611914615)
We could exercise these errors (file doesn't exist and open failure) in the functional tests. An (obvious) unkilled mutant:
```diff
diff --git a/src/init.cpp b/muts-pr-34004-init-cpp/init.mutant.1.cpp
index fb7336f9ae..59de0f6b7a 100644
--- a/src/init.cpp
+++ b/muts-pr-34004-init-cpp/init.mutant.1.cpp
@@ -1812,7 +1812,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
AutoFile afile{file};
if (afile.IsNull()) {
LogError
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611914615)
We could exercise these errors (file doesn't exist and open failure) in the functional tests. An (obvious) unkilled mutant:
```diff
diff --git a/src/init.cpp b/muts-pr-34004-init-cpp/init.mutant.1.cpp
index fb7336f9ae..59de0f6b7a 100644
--- a/src/init.cpp
+++ b/muts-pr-34004-init-cpp/init.mutant.1.cpp
@@ -1812,7 +1812,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
AutoFile afile{file};
if (afile.IsNull()) {
LogError
...
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611893305)
Unkilled mutant:
```diff
diff --git a/src/validation.cpp b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
index 3d84576e7d..efc5037570 100644
--- a/src/validation.cpp
+++ b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
@@ -2613,7 +2613,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
// * legacy (always)
// * p2sh (when P2SH enabled in flags and excludes coinbase)
// * witness (when witness enabled in flags an
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611893305)
Unkilled mutant:
```diff
diff --git a/src/validation.cpp b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
index 3d84576e7d..efc5037570 100644
--- a/src/validation.cpp
+++ b/muts-pr-34004-validation-cpp/validation.mutant.16.cpp
@@ -2613,7 +2613,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
// * legacy (always)
// * p2sh (when P2SH enabled in flags and excludes coinbase)
// * witness (when witness enabled in flags an
...
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611981935)
This error isn't covered by any test and could be addressed in `feature_swiftsync`. Perhaps by deleting a block file before calling the `generatetxohints` RPC.
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611981935)
This error isn't covered by any test and could be addressed in `feature_swiftsync`. Perhaps by deleting a block file before calling the `generatetxohints` RPC.
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611879532)
> these failures could be exercised in the provided unit test
+1. Some unkilled mutants for obvious lack of testing:
```diff
--- a/src/swiftsync.cpp
+++ b/muts-pr-34004-swiftsync-cpp/swiftsync.mutant.25.cpp
@@ -82,7 +82,7 @@ HintsfileReader::HintsfileReader(AutoFile& file) : m_file(file.release())
}
uint8_t version{};
m_file >> version;
- if (version != FILE_VERSION) {
+ if (1==0) {
throw std::ios_base::failure("HintsfileReader: Unsupported file versi
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2611879532)
> these failures could be exercised in the provided unit test
+1. Some unkilled mutants for obvious lack of testing:
```diff
--- a/src/swiftsync.cpp
+++ b/muts-pr-34004-swiftsync-cpp/swiftsync.mutant.25.cpp
@@ -82,7 +82,7 @@ HintsfileReader::HintsfileReader(AutoFile& file) : m_file(file.release())
}
uint8_t version{};
m_file >> version;
- if (version != FILE_VERSION) {
+ if (1==0) {
throw std::ios_base::failure("HintsfileReader: Unsupported file versi
...
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612145025)
nit:
```suggestion
for _ in range(NUM_SWIFTSYNC_BLOCKS):
```
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612145025)
nit:
```suggestion
for _ in range(NUM_SWIFTSYNC_BLOCKS):
```
💬 brunoerg commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612266788)
Unkilled mutant:
```diff
diff --git a/src/swiftsync.h b/muts-pr-34004-swiftsync-h/swiftsync.mutant.0.h
index 2c60e96895..ae229d18d8 100644
--- a/src/swiftsync.h
+++ b/muts-pr-34004-swiftsync-h/swiftsync.mutant.0.h
@@ -36,7 +36,7 @@ private:
public:
Aggregate();
/** Is the internal state zero, representing the empty set. */
- bool IsZero() const { return m_limb0 == 0 && m_limb1 == 0 && m_limb2 == 0 && m_limb3 == 0; }
+ bool IsZero() const { return m_limb0 == 0 || m_lim
...
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2612266788)
Unkilled mutant:
```diff
diff --git a/src/swiftsync.h b/muts-pr-34004-swiftsync-h/swiftsync.mutant.0.h
index 2c60e96895..ae229d18d8 100644
--- a/src/swiftsync.h
+++ b/muts-pr-34004-swiftsync-h/swiftsync.mutant.0.h
@@ -36,7 +36,7 @@ private:
public:
Aggregate();
/** Is the internal state zero, representing the empty set. */
- bool IsZero() const { return m_limb0 == 0 && m_limb1 == 0 && m_limb2 == 0 && m_limb3 == 0; }
+ bool IsZero() const { return m_limb0 == 0 || m_lim
...
🤔 mzumsande reviewed a pull request: "net: Waste less time in socket handling"
(https://github.com/bitcoin/bitcoin/pull/34025#pullrequestreview-3569594119)
ACK 5f5c1ea01955d277581f6c2acbeb982949088960
Looks like an improvement, but I wonder if instead of doing micro-optimizations like the ones here, it wouldn't be better to have the entire `InactivityCheck()` procedure run somewhere else than in the `SocketHandlerConnected`, or at least less often - it seems a bit absurd to check every `50ms` for a timeout of 20 minutes. Even the initial waiting time `m_peer_connect_timeout` from `ShouldRunInactivityChecks()` for a new peer doesn't really be che
...
(https://github.com/bitcoin/bitcoin/pull/34025#pullrequestreview-3569594119)
ACK 5f5c1ea01955d277581f6c2acbeb982949088960
Looks like an improvement, but I wonder if instead of doing micro-optimizations like the ones here, it wouldn't be better to have the entire `InactivityCheck()` procedure run somewhere else than in the `SocketHandlerConnected`, or at least less often - it seems a bit absurd to check every `50ms` for a timeout of 20 minutes. Even the initial waiting time `m_peer_connect_timeout` from `ShouldRunInactivityChecks()` for a new peer doesn't really be che
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3644102756)
I've rebased due to https://github.com/bitcoin/bitcoin/pull/33602, and added several touchups. Thank you @l0rinc for the suggestions!
Thank you also @l0rinc for your very thorough measurements. I think 4 threads is a decent choice for now, but as @TheBlueMatt suggests I will try and run benchmarks in a cloud environment with network connected storage.
I would like to reproduce the memory findings, but https://github.com/bitcoin/bitcoin/issues/33351 makes it a little difficult to determine
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3644102756)
I've rebased due to https://github.com/bitcoin/bitcoin/pull/33602, and added several touchups. Thank you @l0rinc for the suggestions!
Thank you also @l0rinc for your very thorough measurements. I think 4 threads is a decent choice for now, but as @TheBlueMatt suggests I will try and run benchmarks in a cloud environment with network connected storage.
I would like to reproduce the memory findings, but https://github.com/bitcoin/bitcoin/issues/33351 makes it a little difficult to determine
...
💬 stickies-v commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612421141)
nit: for people unfamiliar with these tests, it's not obvious that the relevant debug/category settings are set in `LogSetup`. would be a nice improvement to make the test more self contained by explicitly setting them at the beginning of the test (even if it's duplication)
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612421141)
nit: for people unfamiliar with these tests, it's not obvious that the relevant debug/category settings are set in `LogSetup`. would be a nice improvement to make the test more self contained by explicitly setting them at the beginning of the test (even if it's duplication)
👍 stickies-v approved a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3569753904)
ACK fa35682637e7848106189e314a963d8f6c45b2bc
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3569753904)
ACK fa35682637e7848106189e314a963d8f6c45b2bc
⚠️ memeticae opened an issue: "Bitcoin to send field is nearly invisible"
(https://github.com/bitcoin/bitcoin/issues/34052)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
Bitcoin to send field is nearly invisible, it works, but the up and down arrows completely obfuscate the text area.
(https://github.com/bitcoin/bitcoin/issues/34052)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [x] I still think this issue should be opened here
### Report
Bitcoin to send field is nearly invisible, it works, but the up and down arrows completely obfuscate the text area.
✅ pinheadmz closed an issue: "Bitcoin to send field is nearly invisible"
(https://github.com/bitcoin/bitcoin/issues/34052)
(https://github.com/bitcoin/bitcoin/issues/34052)
💬 pinheadmz commented on issue "Bitcoin to send field is nearly invisible":
(https://github.com/bitcoin/bitcoin/issues/34052#issuecomment-3644359946)
See https://github.com/bitcoin-core/gui/issues/906
(https://github.com/bitcoin/bitcoin/issues/34052#issuecomment-3644359946)
See https://github.com/bitcoin-core/gui/issues/906
💬 pinheadmz commented on issue "Bitcoin to send field is nearly invisible":
(https://github.com/bitcoin/bitcoin/issues/34052#issuecomment-3644362867)
Or otherwise search open issues in https://github.com/bitcoin-core/gui/issues before opening a new issue
(https://github.com/bitcoin/bitcoin/issues/34052#issuecomment-3644362867)
Or otherwise search open issues in https://github.com/bitcoin-core/gui/issues before opening a new issue
🤔 w0xlt reviewed a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3569919476)
ACK https://github.com/bitcoin/bitcoin/pull/34051/commits/fa35682637e7848106189e314a963d8f6c45b2bc
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3569919476)
ACK https://github.com/bitcoin/bitcoin/pull/34051/commits/fa35682637e7848106189e314a963d8f6c45b2bc
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3644700618)
I have added an extensive comment inside the `SpanningForestState::Activate()` function.
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3644700618)
I have added an extensive comment inside the `SpanningForestState::Activate()` function.
🤔 Ataraxia009 reviewed a pull request: "log: Remove brittle and confusing LogPrintLevel"
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3570363089)
Concept Ack
(https://github.com/bitcoin/bitcoin/pull/34051#pullrequestreview-3570363089)
Concept Ack
💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612981132)
Why not write this like?
```
#define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
do {
Assume(level >= BCLog::Level::Info);
if (LogAcceptCategory((category), (level))) {
LogPrintLevel_(category, level, rate_limit, __VA_ARGS__);
}
} while (0)
```
(https://github.com/bitcoin/bitcoin/pull/34051#discussion_r2612981132)
Why not write this like?
```
#define detail_LogIfCategoryAndLevelEnabled(category, level, ...)
do {
Assume(level >= BCLog::Level::Info);
if (LogAcceptCategory((category), (level))) {
LogPrintLevel_(category, level, rate_limit, __VA_ARGS__);
}
} while (0)
```
💬 Ataraxia009 commented on pull request "log: Remove brittle and confusing LogPrintLevel":
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3644985274)
This is good LogPrintLevel just leads to more confusion, standardise the macros!
(https://github.com/bitcoin/bitcoin/pull/34051#issuecomment-3644985274)
This is good LogPrintLevel just leads to more confusion, standardise the macros!
💬 stratospher commented on pull request "p2p: avoid traversing blocks (twice) during IBD":
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3644999563)
planning on picking this up in a separate PR - unless someone already has a branch ready (happy to review in that case!).
(https://github.com/bitcoin/bitcoin/pull/32730#issuecomment-3644999563)
planning on picking this up in a separate PR - unless someone already has a branch ready (happy to review in that case!).
💬 Ataraxia009 commented on pull request "refactor: inline constant `f_obfuscate = false` parameter":
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3645025381)
> if it's needed in the future, it's trivial to reintroduce it.
It seems like a strong enough option to leave as is still but ill defer to others i dont have a strong opinion
(https://github.com/bitcoin/bitcoin/pull/34048#issuecomment-3645025381)
> if it's needed in the future, it's trivial to reintroduce it.
It seems like a strong enough option to leave as is still but ill defer to others i dont have a strong opinion