💬 jarolrod commented on pull request "Adjust plural forms for translations":
(https://github.com/bitcoin-core/gui/pull/716#issuecomment-1478968364)
concept ack
(https://github.com/bitcoin-core/gui/pull/716#issuecomment-1478968364)
concept ack
💬 TheCharlatan commented on pull request "Introduce `MockableDatabase` for wallet unit tests":
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1144287702)
Yeah totally, not sure what I was thinking here.
Please resolve this.
(https://github.com/bitcoin/bitcoin/pull/26715#discussion_r1144287702)
Yeah totally, not sure what I was thinking here.
Please resolve this.
💬 nopara73 commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1479058400)
For Wasabi's purposes this output is needed purely for fee estimation, so it'd be ideal if Core would estimate fees properly. Here's how I see things:
- `estimatesmartfee` estimates fees based on the **PAST**
- This PR enables us to put minimums on Core's fee estimations. Eg. if Core estimates 6 hours at 10 sat/b, based on the past, but the mempool is already filled with 12 hours of 11 sat/b estimations, then what we do (currently with Knots) is that based on the mempool histogram we upgrade
...
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1479058400)
For Wasabi's purposes this output is needed purely for fee estimation, so it'd be ideal if Core would estimate fees properly. Here's how I see things:
- `estimatesmartfee` estimates fees based on the **PAST**
- This PR enables us to put minimums on Core's fee estimations. Eg. if Core estimates 6 hours at 10 sat/b, based on the past, but the mempool is already filled with 12 hours of 11 sat/b estimations, then what we do (currently with Knots) is that based on the mempool histogram we upgrade
...
💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421842)
I can make `chainname` a struct instead and use the same pattern as it was before in `chainparamsbase`, but I'm not sure if this is so much better. Isn't the default string constructor called anyway for each time this header is included? How about taking this a step further and making them a `const std::string_view` instead? I pushed https://github.com/TheCharlatan/bitcoin/commit/1e4ba77db8121f4bc58653c786595325b308ac31 as a proof of concept. Looking at the diff, I'm not sure if it is worth it t
...
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421842)
I can make `chainname` a struct instead and use the same pattern as it was before in `chainparamsbase`, but I'm not sure if this is so much better. Isn't the default string constructor called anyway for each time this header is included? How about taking this a step further and making them a `const std::string_view` instead? I pushed https://github.com/TheCharlatan/bitcoin/commit/1e4ba77db8121f4bc58653c786595325b308ac31 as a proof of concept. Looking at the diff, I'm not sure if it is worth it t
...
💬 TheCharlatan commented on pull request "refactor: Move chain names to the kernel namespace":
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421895)
Thank you for this concrete split suggestion. I did not add a scripted diff, to show the change of header and definition namespacing in the same commit. This makes it easier to see that for every changed file the new header is included as well. To me this feels like it will make for a cleaner commit history. However, I'm new to contributing and reviewing large code moves to the project. I'll ponder over your suggested split, I usually come around to these suggestions after a bit :)
(https://github.com/bitcoin/bitcoin/pull/27294#discussion_r1144421895)
Thank you for this concrete split suggestion. I did not add a scripted diff, to show the change of header and definition namespacing in the same commit. This makes it easier to see that for every changed file the new header is included as well. To me this feels like it will make for a cleaner commit history. However, I'm new to contributing and reviewing large code moves to the project. I'll ponder over your suggested split, I usually come around to these suggestions after a bit :)
💬 S3RK commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1144434340)
I realised my mistake. You can resolve this and ignore my comments. I'll go over the code one more time before I ack
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1144434340)
I realised my mistake. You can resolve this and ignore my comments. I'll go over the code one more time before I ack
💬 0xB10C commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144467948)
Thanks, helpful comments!
- `m_addr_name`: my reasoning for passing `m_addr_name` as string instead of binary is that we likely don't want to implement the binary decoding in each tracing script. I think the string is closer to what users need. However, not opposing to add the binary if there's need.
- `nKeyedNetGroup`: I added the netgroup as argument to better identify connections coming from the same network. For my use-case, it's not problematic if `nKeyedNetGroup` isn't consistent acros
...
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144467948)
Thanks, helpful comments!
- `m_addr_name`: my reasoning for passing `m_addr_name` as string instead of binary is that we likely don't want to implement the binary decoding in each tracing script. I think the string is closer to what users need. However, not opposing to add the binary if there's need.
- `nKeyedNetGroup`: I added the netgroup as argument to better identify connections coming from the same network. For my use-case, it's not problematic if `nKeyedNetGroup` isn't consistent acros
...
💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457784)
Can you apply the changes to the other loops too? i.e.
```suggestion
for (size_t put_index{0}; put_index < num_puts; ++put_index) {
```
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457784)
Can you apply the changes to the other loops too? i.e.
```suggestion
for (size_t put_index{0}; put_index < num_puts; ++put_index) {
```
💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144470215)
Wait, if you're using `txns[put_index]` *and* you're using the same `txns` reference each time you call `add_parents_child`, aren't you having all subsequent packages spend outputs from txns[0:24] too? That means all these packages conflict with each other.
You could fix this by adding `set_num * package_size` or something to the index. But what would probably make a better interface is if `add_parents_child` returns the list of transactions it creates, and you append them to your larger list
...
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144470215)
Wait, if you're using `txns[put_index]` *and* you're using the same `txns` reference each time you call `add_parents_child`, aren't you having all subsequent packages spend outputs from txns[0:24] too? That means all these packages conflict with each other.
You could fix this by adding `set_num * package_size` or something to the index. But what would probably make a better interface is if `add_parents_child` returns the list of transactions it creates, and you append them to your larger list
...
💬 glozow commented on pull request "bench: Expand mempool eviction benchmarking":
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457060)
Didn't remove?
(https://github.com/bitcoin/bitcoin/pull/27019#discussion_r1144457060)
Didn't remove?
💬 MarcoFalke commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144475716)
```suggestion
Ticks<std::chrono::seconds>(m_connected);
```
nit, use `Ticks`, or `count_seconds` (if you want the compile failure if m_connected is changed to higher precision than seconds)?
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1144475716)
```suggestion
Ticks<std::chrono::seconds>(m_connected);
```
nit, use `Ticks`, or `count_seconds` (if you want the compile failure if m_connected is changed to higher precision than seconds)?
🚀 fanquake merged a pull request: "Refactor: Remove unused FlatFilePos::SetNull"
(https://github.com/bitcoin/bitcoin/pull/27289)
(https://github.com/bitcoin/bitcoin/pull/27289)
💬 fanquake commented on pull request "Refactor: Remove unused FlatFilePos::SetNull":
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1479227767)
Door open for anyone to followup with more `::SetNull()` removal.
(https://github.com/bitcoin/bitcoin/pull/27289#issuecomment-1479227767)
Door open for anyone to followup with more `::SetNull()` removal.
💬 fanquake commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479232890)
> What is the combined log?
Unfortunately don't have one, and cannot seem to recreate this failure. Happy to close until have more useful info.
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479232890)
> What is the combined log?
Unfortunately don't have one, and cannot seem to recreate this failure. Happy to close until have more useful info.
💬 MarcoFalke commented on issue "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`":
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479235761)
ok. Let's reopen then
(https://github.com/bitcoin/bitcoin/issues/27281#issuecomment-1479235761)
ok. Let's reopen then
✅ MarcoFalke closed an issue: "test: `interface_bitcoin_cli.py --descriptors` failure under `--valgrind`"
(https://github.com/bitcoin/bitcoin/issues/27281)
(https://github.com/bitcoin/bitcoin/issues/27281)
💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1479241540)
re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 95ad70ab652ddde7de65
...
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1479241540)
re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 95ad70ab652ddde7de65
...
🚀 fanquake merged a pull request: "test: Fix TypeError (expected str instance, bytes found) in wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/27280)
(https://github.com/bitcoin/bitcoin/pull/27280)
📝 fanquake opened a pull request: "guix: import/sync python-lief (0.12.3) package definition from upstream"
(https://github.com/bitcoin/bitcoin/pull/27296)
Update to version 0.12.3.
Retain our PPC64 patch.
Mention when we can drop our local definition.
Also switch to using cmake-minimal (see #27172), which fixes atleast one build failure I've seen on aarch64, where cmake dependencies fail to build. Fix that by using the cmake without all the dependencies we don't actually need:
```bash
The following derivations will be built:
/gnu/store/7qqvqq2g7l5ylrjv0gn6zha565a12kar-python-lief-0.12.1.drv
/gnu/store/f9zwh1ldy63ga0i5w6cbbqlj6sfq226j-
...
(https://github.com/bitcoin/bitcoin/pull/27296)
Update to version 0.12.3.
Retain our PPC64 patch.
Mention when we can drop our local definition.
Also switch to using cmake-minimal (see #27172), which fixes atleast one build failure I've seen on aarch64, where cmake dependencies fail to build. Fix that by using the cmake without all the dependencies we don't actually need:
```bash
The following derivations will be built:
/gnu/store/7qqvqq2g7l5ylrjv0gn6zha565a12kar-python-lief-0.12.1.drv
/gnu/store/f9zwh1ldy63ga0i5w6cbbqlj6sfq226j-
...
💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693)
Same commit: Can remove the now unused constructors for all other structs as well? See diff, plus unrelated cleanup: (Maybe for a follow-up to avoid another round of re-review?)
```diff
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 012b0eb321..975dace633 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -57,7 +57,6 @@ struct FakeCheckCheckCompletion {
struct FailingCheck {
bool fails;
FailingCheck(bool _fa
...
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693)
Same commit: Can remove the now unused constructors for all other structs as well? See diff, plus unrelated cleanup: (Maybe for a follow-up to avoid another round of re-review?)
```diff
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 012b0eb321..975dace633 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -57,7 +57,6 @@ struct FakeCheckCheckCompletion {
struct FailingCheck {
bool fails;
FailingCheck(bool _fa
...