🤔 sipa reviewed a pull request: "cluster mempool: add TxGraph work controls"
(https://github.com/bitcoin/bitcoin/pull/32263#pullrequestreview-3016660633)
Addressed comments, and also made some further simplifications (dropped the returning of amount of work performed by `MakeAcceptable` and `MakeAllAcceptable` in the preparation commit, as the new `DoWork()` doesn't use them anymore but invokes `Cluster::Relinearize` directly).
(https://github.com/bitcoin/bitcoin/pull/32263#pullrequestreview-3016660633)
Addressed comments, and also made some further simplifications (dropped the returning of amount of work performed by `MakeAcceptable` and `MakeAllAcceptable` in the preparation commit, as the new `DoWork()` doesn't use them anymore but invokes `Cluster::Relinearize` directly).
💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205126924)
I have added some additional comments.
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205126924)
I have added some additional comments.
💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205127512)
Done.
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205127512)
Done.
💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205127983)
Added some comments to clarify.
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205127983)
Added some comments to clarify.
✅ maflcko closed a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936)
(https://github.com/bitcoin/bitcoin/pull/31936)
💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3069899126)
Closing for now, due to inactivity. The given feedback hasn't bee addressed over a long time and at this point it may be best to focus review on https://github.com/bitcoin/bitcoin/pull/32896.
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3069899126)
Closing for now, due to inactivity. The given feedback hasn't bee addressed over a long time and at this point it may be best to focus review on https://github.com/bitcoin/bitcoin/pull/32896.
💬 josibake commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069964449)
> > am not sure how I am to interpret your NACK here.
>
> Just as "I think this PR should not be merged in its current form." I definitely do agree with the approach of adding a C API.
FWIW, I read this as "Concept ACK, Approach NACK" (per https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review), which I think is a helpful distinction.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069964449)
> > am not sure how I am to interpret your NACK here.
>
> Just as "I think this PR should not be merged in its current form." I definitely do agree with the approach of adding a C API.
FWIW, I read this as "Concept ACK, Approach NACK" (per https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review), which I think is a helpful distinction.
💬 maflcko commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3069982232)
review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣
<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: review ACK c18bf0bd9be6
...
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3069982232)
review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣
<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: review ACK c18bf0bd9be6
...
💬 purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069994277)
Yes, Concept ACK, Approach NACK
Thanks, @josibake.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069994277)
Yes, Concept ACK, Approach NACK
Thanks, @josibake.
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3070013951)
Updated 267a7b3f321304f75e8c47e380da49ba9c64bc84 -> 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe ([kernelApi_44](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_44) -> [kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_44..kernelApi_45))
* Enforce better function names in the API, which should make future discussions on their desired end format a bit easier.
* Dropped the macro check for gcc 4.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3070013951)
Updated 267a7b3f321304f75e8c47e380da49ba9c64bc84 -> 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe ([kernelApi_44](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_44) -> [kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_44..kernelApi_45))
* Enforce better function names in the API, which should make future discussions on their desired end format a bit easier.
* Dropped the macro check for gcc 4.
🚀 fanquake merged a pull request: "refactor: cleanup index logging"
(https://github.com/bitcoin/bitcoin/pull/32948)
(https://github.com/bitcoin/bitcoin/pull/32948)
🤔 mzumsande reviewed a pull request: "test: fix intermittent failure in rpc_invalidateblock.py"
(https://github.com/bitcoin/bitcoin/pull/32968#pullrequestreview-3016812338)
Code Review ACK 28416f367a5d720d89214aa8ef94013caa1d1a40
(https://github.com/bitcoin/bitcoin/pull/32968#pullrequestreview-3016812338)
Code Review ACK 28416f367a5d720d89214aa8ef94013caa1d1a40
💬 m3dwards commented on pull request "test: use notarized v28.2 binaries and fix macOS detection":
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3070026124)
ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9
Tested that the correct binaries were self signed on ARM Mac. Also checked the SHA256 sums for 28.2.
(https://github.com/bitcoin/bitcoin/pull/32922#issuecomment-3070026124)
ACK 4bb4c865999bfa0b0cb5aa806cf62dbf982fcfe9
Tested that the correct binaries were self signed on ARM Mac. Also checked the SHA256 sums for 28.2.
📝 crypomen9 opened a pull request: "chore: rename misspelling variable"
(https://github.com/bitcoin/bitcoin/pull/32971)
### Description
This pull request refactors the variable name from `shuffled_indeces` to `shuffled_indices` for improved code readability and consistency with standard naming conventions.
### Impact
- No changes to logic or functionality.
- No new tests are required, as this is a non-functional refactor.
- This change improves the developer experience by making the codebase easier to read and maintain.
(https://github.com/bitcoin/bitcoin/pull/32971)
### Description
This pull request refactors the variable name from `shuffled_indeces` to `shuffled_indices` for improved code readability and consistency with standard naming conventions.
### Impact
- No changes to logic or functionality.
- No new tests are required, as this is a non-functional refactor.
- This change improves the developer experience by making the codebase easier to read and maintain.
💬 anhilde commented on issue "Bitcoin Core v29.0 incorrectly enters IBD mode when only ~600 blocks behind, preventing normal sync":
(https://github.com/bitcoin/bitcoin/issues/32955#issuecomment-3070086012)
ok, I will look out for that the next time. I am pretty sure the node was up a few hours before I noticed that the block number had not changed. What is the normal time until a node must have resumed syncing again? Is there a timeframe where I can be sure something is wrong?
(https://github.com/bitcoin/bitcoin/issues/32955#issuecomment-3070086012)
ok, I will look out for that the next time. I am pretty sure the node was up a few hours before I noticed that the block number had not changed. What is the normal time until a node must have resumed syncing again? Is there a timeframe where I can be sure something is wrong?
💬 sipa commented on issue "Bitcoin Core v29.0 incorrectly enters IBD mode when only ~600 blocks behind, preventing normal sync":
(https://github.com/bitcoin/bitcoin/issues/32955#issuecomment-3070117257)
It should literally *always* be syncing. If it doesn't pick up an announced block immediately (within seconds, at the very longest), there is something wrong.
(https://github.com/bitcoin/bitcoin/issues/32955#issuecomment-3070117257)
It should literally *always* be syncing. If it doesn't pick up an announced block immediately (within seconds, at the very longest), there is something wrong.
💬 1440000bytes commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3070190437)
Better to admit that USD is still the currency used for payments and there isn't any difference from 1 USD in 2015 and 2025.
Maybe Bitcoin(protocol) is dependent on USD(US government)?
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3070190437)
Better to admit that USD is still the currency used for payments and there isn't any difference from 1 USD in 2015 and 2025.
Maybe Bitcoin(protocol) is dependent on USD(US government)?
🤔 Sjors reviewed a pull request: "descriptors: MuSig2"
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-3016814938)
Reviewed from 4af0dca096ca497a6b4e5314c9edea683efe620e to a53924bee321f9d01d053cf562ee3d9493e00529.
In a53924bee321f9d01d053cf562ee3d9493e00529 _descriptor: Parse musig() key expressions_: it might be slightly easier to follow if you extract `& has_hardened`, the `ParsePubkey` signature change and `FromString` changes into their own commit(s).
(https://github.com/bitcoin/bitcoin/pull/31244#pullrequestreview-3016814938)
Reviewed from 4af0dca096ca497a6b4e5314c9edea683efe620e to a53924bee321f9d01d053cf562ee3d9493e00529.
In a53924bee321f9d01d053cf562ee3d9493e00529 _descriptor: Parse musig() key expressions_: it might be slightly easier to follow if you extract `& has_hardened`, the `ParsePubkey` signature change and `FromString` changes into their own commit(s).
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2205328232)
In a53924bee321f9d01d053cf562ee3d9493e00529 _descriptor: Parse musig() key expressions_: I'm confused by how this variable starts as `true`.
```diff
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1843,20 +1843,20 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
bool any_ranged = false;
bool all_bip32 = true;
std::vector<std::vector<std::unique_ptr<PubkeyProvider>>> providers;
- bool any_key_parsed
...
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2205328232)
In a53924bee321f9d01d053cf562ee3d9493e00529 _descriptor: Parse musig() key expressions_: I'm confused by how this variable starts as `true`.
```diff
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1843,20 +1843,20 @@ std::vector<std::unique_ptr<PubkeyProvider>> ParsePubkey(uint32_t& key_exp_index
bool any_ranged = false;
bool all_bip32 = true;
std::vector<std::vector<std::unique_ptr<PubkeyProvider>>> providers;
- bool any_key_parsed
...
💬 Sjors commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2205223219)
And if there accidentally was / becomes a way, hopefully a fuzzer catches it with the `Assume` above.
(in 4af0dca096ca497a6b4e5314c9edea683efe620e _descriptor: Add MuSigPubkeyProvider_)
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2205223219)
And if there accidentally was / becomes a way, hopefully a fuzzer catches it with the `Assume` above.
(in 4af0dca096ca497a6b4e5314c9edea683efe620e _descriptor: Add MuSigPubkeyProvider_)