💬 optout21 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2526627259)
To be safe, I suggest keeping the output here as it was, as I have no idea what was the rationale for it: just so happened, or to reflect that segwit segregation.
A way to do it is to have a version of Tx tostring that omits the witness, and use that here, and print the witnesses separately as before.
I think this is less intrusive change and higher change of positive review.
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2526627259)
To be safe, I suggest keeping the output here as it was, as I have no idea what was the rationale for it: just so happened, or to reflect that segwit segregation.
A way to do it is to have a version of Tx tostring that omits the witness, and use that here, and print the witnesses separately as before.
I think this is less intrusive change and higher change of positive review.
💬 optout21 commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3531792562)
> we're already constrained on review, and should therefore be prioritising what review resources we have to the most valuable PRs we have, not the easiest ones. This PR in particular, and the strategy of "let's split off easy commits into separate PRs" more generally, does the opposite
I get your points, however, to nuance it a bit:
Splitting of smaller parts into breakout PRs:
Cons:
- higher total overhead
- may draw review resources from more important PRs
- risk of shallow review as
...
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3531792562)
> we're already constrained on review, and should therefore be prioritising what review resources we have to the most valuable PRs we have, not the easiest ones. This PR in particular, and the strategy of "let's split off easy commits into separate PRs" more generally, does the opposite
I get your points, however, to nuance it a bit:
Splitting of smaller parts into breakout PRs:
Cons:
- higher total overhead
- may draw review resources from more important PRs
- risk of shallow review as
...
💬 optout21 commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3531799884)
reACK 78595ae0e71b833ebe7640d5120eea5da14f55ab
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3531799884)
reACK 78595ae0e71b833ebe7640d5120eea5da14f55ab
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3531815161)
`36ae854b1d...3e6b5a9ec4`: take https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2525677364
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3531815161)
`36ae854b1d...3e6b5a9ec4`: take https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2525677364
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2526732684)
Amazing! TIL atomics can be used as condition variables (since C++20). It is simpler this way, thanks!
I copied the above verbatim, so put you as co-author of that commit.
My only "review" nit is that here:
```cpp
size_t current_value{0};
size_t new_value{0};
do {
current_value = ...
new_value = ...
} while ...
```
the initialization of the two variables to `0` can be omitted since they are assigned unconditionally right af
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2526732684)
Amazing! TIL atomics can be used as condition variables (since C++20). It is simpler this way, thanks!
I copied the above verbatim, so put you as co-author of that commit.
My only "review" nit is that here:
```cpp
size_t current_value{0};
size_t new_value{0};
do {
current_value = ...
new_value = ...
} while ...
```
the initialization of the two variables to `0` can be omitted since they are assigned unconditionally right af
...
💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2526743891)
Isn't it cleaner to just drop the try/catch entirely in that case?
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2526743891)
Isn't it cleaner to just drop the try/catch entirely in that case?
💬 l0rinc commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2526752792)
The suggested comparator is a pure function, I don't see how it could be non-deterministic - unless it's broken, in which case we should definitely dig deeper.
Can you try it again an report what you see?
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2526752792)
The suggested comparator is a pure function, I don't see how it could be non-deterministic - unless it's broken, in which case we should definitely dig deeper.
Can you try it again an report what you see?
🚀 fanquake merged a pull request: "depends: drop Qt patches"
(https://github.com/bitcoin/bitcoin/pull/33860)
(https://github.com/bitcoin/bitcoin/pull/33860)
👍 TheCharlatan approved a pull request: "depends: sqlite 3.51.0; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655#pullrequestreview-3464109401)
ACK 1db74914706fcfafb22646288458604a4a7b6282
Guix build:
```
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21bf1d78cd8ebe9719b1 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706
...
(https://github.com/bitcoin/bitcoin/pull/32655#pullrequestreview-3464109401)
ACK 1db74914706fcfafb22646288458604a4a7b6282
Guix build:
```
3e2443f9828f1541c02f5c33d29c78945bf218411bc35cf13894ad048495aad2 guix-build-1db74914706f/output/aarch64-linux-gnu/SHA256SUMS.part
4a0f92a547fb3f94f930cbdb0575aaab16b1f7d95cd9172cce44068e1ec7ff62 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706f-aarch64-linux-gnu-debug.tar.gz
c98f76eae0da3bf25774f5e8b25edac8d315bc428b2e21bf1d78cd8ebe9719b1 guix-build-1db74914706f/output/aarch64-linux-gnu/bitcoin-1db74914706
...
👍 rkrux approved a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464238462)
lgtm ACK 0aebdac95da9a7d476264424c0107bd806ce5362
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464238462)
lgtm ACK 0aebdac95da9a7d476264424c0107bd806ce5362
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3532063730)
I was still wondering how number of parallel threads affects this, given that it's not a cpu-bound task.
The measurements were done on i9, m4 and rpi4. First two have 16 threads, rpi has 4.
The results still indicate to me that it doesn't make sense to set parallelism based on number of cpus directly. Beyond 4-8 threads the systems didn't really perform any better.
<img width="1489" height="861" alt="image" src="https://github.com/user-attachments/assets/c6065154-0625-4609-928f-867c957ba2
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3532063730)
I was still wondering how number of parallel threads affects this, given that it's not a cpu-bound task.
The measurements were done on i9, m4 and rpi4. First two have 16 threads, rpi has 4.
The results still indicate to me that it doesn't make sense to set parallelism based on number of cpus directly. Beyond 4-8 threads the systems didn't really perform any better.
<img width="1489" height="861" alt="image" src="https://github.com/user-attachments/assets/c6065154-0625-4609-928f-867c957ba2
...
📝 Sjors converted_to_draft a pull request: "mining: add getCoinbase()"
(https://github.com/bitcoin/bitcoin/pull/33819)
Deprecate `getCoinbaseTx()` in favor of a new method that provides a struct with everything clients need to construct a coinbase. This is safer than providing a raw dummy coinbase that clients then have to manipulate.
Also deprecate `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.
A new helper method `ExtractCoinbaseTemplate()` processes the dummy coinbase transaction generated by `BlockAssembler::CreateNewBlock` and produces a `CoinbaseTemplate`.
Expand the `interface_ipc.p
...
(https://github.com/bitcoin/bitcoin/pull/33819)
Deprecate `getCoinbaseTx()` in favor of a new method that provides a struct with everything clients need to construct a coinbase. This is safer than providing a raw dummy coinbase that clients then have to manipulate.
Also deprecate `getCoinbaseCommitment()` and `getWitnessCommitmentIndex()`.
A new helper method `ExtractCoinbaseTemplate()` processes the dummy coinbase transaction generated by `BlockAssembler::CreateNewBlock` and produces a `CoinbaseTemplate`.
Expand the `interface_ipc.p
...
💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532157521)
From https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008 I gather that we should not reuse the method ordinals. Since cap&proto doesn't allow gaps, when deprecating methods, it's best to just have them throw an error.
In https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3529071835 @ryanofsky suggested replacing the dummy coinbase with an empty transaction, so that clients are not tempted to use the dummy coinbase via `getBlock()` or `getCoinbaseTx()`.
I think
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532157521)
From https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3528997008 I gather that we should not reuse the method ordinals. Since cap&proto doesn't allow gaps, when deprecating methods, it's best to just have them throw an error.
In https://github.com/bitcoin/bitcoin/issues/33777#issuecomment-3529071835 @ryanofsky suggested replacing the dummy coinbase with an empty transaction, so that clients are not tempted to use the dummy coinbase via `getBlock()` or `getCoinbaseTx()`.
I think
...
🤔 frankomosh reviewed a pull request: "net: Filter addrman during address selection via AddrPolicy to avoid underfill"
(https://github.com/bitcoin/bitcoin/pull/33663#pullrequestreview-3464332521)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33663#pullrequestreview-3464332521)
Concept ACK
💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527033639)
Is there a way/test to verify that the `BanMan` methods here are safe to call while `AddrMan`'s lock is held?
Can see a comment in `addrman.h` says the policy "must not acquire locks that can conflict with `AddrMan::cs`". So it would make sense to ensure deadlock scenario is not accidentally created.
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527033639)
Is there a way/test to verify that the `BanMan` methods here are safe to call while `AddrMan`'s lock is held?
Can see a comment in `addrman.h` says the policy "must not acquire locks that can conflict with `AddrMan::cs`". So it would make sense to ensure deadlock scenario is not accidentally created.
💬 frankomosh commented on pull request "net: Filter addrman during address selection via AddrPolicy to avoid underfill":
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527062353)
Would network mismatches also count as `filtered` i this case?
(https://github.com/bitcoin/bitcoin/pull/33663#discussion_r2527062353)
Would network mismatches also count as `filtered` i this case?
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2527112930)
Ah yes, that is cleaner.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2527112930)
Ah yes, that is cleaner.
💬 yancyribbens commented on pull request "kernel: handle null or empty directories in implementation":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527173880)
> that's literally English for data_dir == nullptr && data_dir_len > 0 evaluating to true, therefore satisfying the if condition, therefore triggering the early return.
The `&&` requires both of conditions to be true to process the early return branch? The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero. So, either `data_dir == nullptr` alone is enough, or `data_dir == nullptr || data_dir_len == 0` to be
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2527173880)
> that's literally English for data_dir == nullptr && data_dir_len > 0 evaluating to true, therefore satisfying the if condition, therefore triggering the early return.
The `&&` requires both of conditions to be true to process the early return branch? The way you had it written before is confusing because if the ptr is null, it is a logical conclusion that data_dir_len should also be zero. So, either `data_dir == nullptr` alone is enough, or `data_dir == nullptr || data_dir_len == 0` to be
...
👍 stickies-v approved a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464527742)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
(https://github.com/bitcoin/bitcoin/pull/33872#pullrequestreview-3464527742)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
💬 TheCharlatan commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527219768)
I don't think it does, afaik a write exception through `Sync` or `Flush` is just bubbled up and then logged. But I might be missing something.
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2527219768)
I don't think it does, afaik a write exception through `Sync` or `Flush` is just bubbled up and then logged. But I might be missing something.
💬 ryanofsky commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532419886)
> I think I'm going to combine these two observations and make this PR a breaking change. Marking as draft for now.
This seems ok, but isn't really what I would suggest. My suggestion would be for this PR to just introduce a new `getCoinBase()` method like it's doing and not remove other methods so existing mining clients do not immediately stop working. The other methods, which are all simple accessors, can easily be removed later.
Adding a new method now and removing older methods later
...
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3532419886)
> I think I'm going to combine these two observations and make this PR a breaking change. Marking as draft for now.
This seems ok, but isn't really what I would suggest. My suggestion would be for this PR to just introduce a new `getCoinBase()` method like it's doing and not remove other methods so existing mining clients do not immediately stop working. The other methods, which are all simple accessors, can easily be removed later.
Adding a new method now and removing older methods later
...