💬 danielabrozzoni commented on pull request "util: Allow `Assert` (et al.) in contexts without __func__":
(https://github.com/bitcoin/bitcoin/pull/33785#issuecomment-3530733454)
post-merge concept ACK, thanks for working on this! :)
(https://github.com/bitcoin/bitcoin/pull/33785#issuecomment-3530733454)
post-merge concept ACK, thanks for working on this! :)
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2525677364)
Would it be simpler to not bother with the mutex and condvar here?
```diff
diff --git a/src/net.cpp b/src/net.cpp
index cd4bcf0ea3..16b2d61b60 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -3095,31 +3095,29 @@ std::optional<Network> CConnman::PrivateBroadcast::PickNetwork(std::optional<Pro
size_t CConnman::PrivateBroadcast::NumToOpen() const
{
- LOCK(m_num_to_open_mutex);
return m_num_to_open;
}
void CConnman::PrivateBroadcast::NumToOpenAdd(size_t n)
{
- WIT
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2525677364)
Would it be simpler to not bother with the mutex and condvar here?
```diff
diff --git a/src/net.cpp b/src/net.cpp
index cd4bcf0ea3..16b2d61b60 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -3095,31 +3095,29 @@ std::optional<Network> CConnman::PrivateBroadcast::PickNetwork(std::optional<Pro
size_t CConnman::PrivateBroadcast::NumToOpen() const
{
- LOCK(m_num_to_open_mutex);
return m_num_to_open;
}
void CConnman::PrivateBroadcast::NumToOpenAdd(size_t n)
{
- WIT
...
💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2525874952)
Maybe (not tested):
```cpp
int Step()
{
const int ret = sqlite3_step(m_stmt);
if (ret == SQLITE_ROW || ret == SQLITE_DONE) {
return ret;
}
sqlite3_finalize(m_stmt);
m_stmt = nullptr;
// then the destructor can assert(sqlite3_finalize(m_stmt) == SQLITE_OK);
}
```
Related to https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518504627
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2525874952)
Maybe (not tested):
```cpp
int Step()
{
const int ret = sqlite3_step(m_stmt);
if (ret == SQLITE_ROW || ret == SQLITE_DONE) {
return ret;
}
sqlite3_finalize(m_stmt);
m_stmt = nullptr;
// then the destructor can assert(sqlite3_finalize(m_stmt) == SQLITE_OK);
}
```
Related to https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518504627
💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2525904085)
I see your point, but the underlying structures are different for `ScriptSig` and `ScriptWitness`: simple byte vector for script sig `ScriptSig` and vector of byte vectors for the `ScriptWitness`. So these are more accurate representatives of what is happening under the hood imo. Its also standard to print script sig like this from what i can see.
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2525904085)
I see your point, but the underlying structures are different for `ScriptSig` and `ScriptWitness`: simple byte vector for script sig `ScriptSig` and vector of byte vectors for the `ScriptWitness`. So these are more accurate representatives of what is happening under the hood imo. Its also standard to print script sig like this from what i can see.
💬 Ataraxia009 commented on pull request "transaction: Adding script witness to ToString for CTxIn":
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2525907152)
yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.
(https://github.com/bitcoin/bitcoin/pull/33711#discussion_r2525907152)
yes but its better to roll up responsibility to the object that holds it? Can you point out exactly where maybe? so we can see? if not, this seems like a better approach to me.
💬 Ataraxia009 commented on pull request "init: Changing the rpcbind argument being ignored to a pop up warning":
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2525940186)
Not necessary since its a weird case imo? Why would you ever hit it?
(https://github.com/bitcoin/bitcoin/pull/33813#discussion_r2525940186)
Not necessary since its a weird case imo? Why would you ever hit it?
💬 willcl-ark commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3531628099)
Updated build:
```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b8
...
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3531628099)
Updated build:
```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b8
...
👍 willcl-ark approved a pull request: "guix: build `bitcoin-qt` with static libxcb & utils"
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3463720384)
utACK 96963b888e5a10f4024fa0449c60c02e3bed6245
The changes here LGTM. I didn't test the built `bitcoin-qt` bin at this time as I have forgotten the incantation needed to do so on NixOS/Wayland at the moment 😢
I did double check the dependencies matched what symbol check is seeing:
<details>
<summary>master</summary>
```
❯ ldd build/bin/bitcoin-qt
linux-vdso.so.1 (0x00007f246b76e000)
libfontconfig.so.1 => not found
libfreetype.so.6 => not found
...
(https://github.com/bitcoin/bitcoin/pull/33537#pullrequestreview-3463720384)
utACK 96963b888e5a10f4024fa0449c60c02e3bed6245
The changes here LGTM. I didn't test the built `bitcoin-qt` bin at this time as I have forgotten the incantation needed to do so on NixOS/Wayland at the moment 😢
I did double check the dependencies matched what symbol check is seeing:
<details>
<summary>master</summary>
```
❯ ldd build/bin/bitcoin-qt
linux-vdso.so.1 (0x00007f246b76e000)
libfontconfig.so.1 => not found
libfreetype.so.6 => not found
...
💬 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
...