π TheCharlatan approved a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753#pullrequestreview-3404408458)
ACK 78d4d36730d44de93690b43f900a9202517291f6
(https://github.com/bitcoin/bitcoin/pull/33753#pullrequestreview-3404408458)
ACK 78d4d36730d44de93690b43f900a9202517291f6
π roconnor-blockstream opened a pull request: "Relax standardness rules regarding CHECKMULTISIG"
(https://github.com/bitcoin/bitcoin/pull/33755)
In PR #5247, the STRICTENC standardness rules were tightned with regards to CHECKMULTISIG so that unparsable public keys fail the script when they are encountered. The overall purpose here was to disallow the use of confusing hybrid public keys by policy while remaining keeping policy compatible (i.e. strictly stronger) than consensus rules.
Comments in PR #5247 note that "I don't believe it should affect any system in production", however this believe is/was false. Counterparty was stuffing
...
(https://github.com/bitcoin/bitcoin/pull/33755)
In PR #5247, the STRICTENC standardness rules were tightned with regards to CHECKMULTISIG so that unparsable public keys fail the script when they are encountered. The overall purpose here was to disallow the use of confusing hybrid public keys by policy while remaining keeping policy compatible (i.e. strictly stronger) than consensus rules.
Comments in PR #5247 note that "I don't believe it should affect any system in production", however this believe is/was false. Counterparty was stuffing
...
π¬ ryanofsky commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473238238)
re: https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3472631221
> Can there ever only be one `waitNext()` call on per `BlockTemplate`? If not, does `interruptWait()` just stop all of them?
There can be multiple waitNext() calls and the current implementation of interruptWait() will interrupt exactly one of them arbitrarily. I speculated on how this could be improved in https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3453225085, if there is a use-case for multiple waitNex
...
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473238238)
re: https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3472631221
> Can there ever only be one `waitNext()` call on per `BlockTemplate`? If not, does `interruptWait()` just stop all of them?
There can be multiple waitNext() calls and the current implementation of interruptWait() will interrupt exactly one of them arbitrarily. I speculated on how this could be improved in https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3453225085, if there is a use-case for multiple waitNex
...
π¬ Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473263150)
I also can't think of a use case, but maybe we should fail if a client (accidentally) tries?
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473263150)
I also can't think of a use case, but maybe we should fail if a client (accidentally) tries?
π€ mzumsande reviewed a pull request: "doc: document fingerprinting risk when operating node on multiple networks"
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3404398572)
Concept ACK
I think that fingerprinting attacks are easy enough (and some, such as https://github.com/bitcoin/bitcoin/pull/33498, are hard to fix) that this warning is justified.
Fingerprinting methods have never been researched systematically as far as I know, so I'm sure there are various unknown ones besides the ones publicly and privately known. Newly found fingerprinting methods aren't classified as CVEs but reported and fixed openly. So this is just a reflection of the status quo.
...
(https://github.com/bitcoin/bitcoin/pull/33750#pullrequestreview-3404398572)
Concept ACK
I think that fingerprinting attacks are easy enough (and some, such as https://github.com/bitcoin/bitcoin/pull/33498, are hard to fix) that this warning is justified.
Fingerprinting methods have never been researched systematically as far as I know, so I'm sure there are various unknown ones besides the ones publicly and privately known. Newly found fingerprinting methods aren't classified as CVEs but reported and fixed openly. So this is just a reflection of the status quo.
...
π¬ mzumsande commented on pull request "doc: document fingerprinting risk when operating node on multiple networks":
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2481496048)
maybe add something like "... and therefore helps strengthen the network" to stress that it's also common to be a bridge node for altruistic reasons, even if the node operator isn't particularly concerned about attacks on their own node.
(https://github.com/bitcoin/bitcoin/pull/33750#discussion_r2481496048)
maybe add something like "... and therefore helps strengthen the network" to stress that it's also common to be a bridge node for altruistic reasons, even if the node operator isn't particularly concerned about attacks on their own node.
π€ rkrux reviewed a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3404260829)
Started reviewing; I'm not opposed to this introduction at the moment and I will get more clarity soon in the review.
Initial Concept ACK 1829b3512d7bf430ec44daa801eb8fadf46cd30b
These 3 commits from #33034 looks pretty generic and related enough. Can they be included in this PR?
- 818989093d9e6b8ee765c21630f7342b99af7fa0 "sqlite: Make SQLiteStatement::Bind a template function"
- 07cac74e1340c7e771850633dd9418ff3efa5bb7 "sqlite: Make SQLiteDatabase::Column an std::optional"
- ff47eea527e58071b
...
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3404260829)
Started reviewing; I'm not opposed to this introduction at the moment and I will get more clarity soon in the review.
Initial Concept ACK 1829b3512d7bf430ec44daa801eb8fadf46cd30b
These 3 commits from #33034 looks pretty generic and related enough. Can they be included in this PR?
- 818989093d9e6b8ee765c21630f7342b99af7fa0 "sqlite: Make SQLiteStatement::Bind a template function"
- 07cac74e1340c7e771850633dd9418ff3efa5bb7 "sqlite: Make SQLiteDatabase::Column an std::optional"
- ff47eea527e58071b
...
π¬ rkrux commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2481404565)
In d13ffed94ad5f5d488f8d72e4d5d7f1c3fb27c2b "sqlite: Have SQLiteCursor store SQLiteStatement"
It doesn't seem to be used anymore here; builds and tests fine without it.
```diff
--- a/src/wallet/sqlite.h
+++ b/src/wallet/sqlite.h
@@ -12,7 +12,6 @@
struct bilingual_str;
-struct sqlite3_stmt;
struct sqlite3;
namespace wallet {
```
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2481404565)
In d13ffed94ad5f5d488f8d72e4d5d7f1c3fb27c2b "sqlite: Have SQLiteCursor store SQLiteStatement"
It doesn't seem to be used anymore here; builds and tests fine without it.
```diff
--- a/src/wallet/sqlite.h
+++ b/src/wallet/sqlite.h
@@ -12,7 +12,6 @@
struct bilingual_str;
-struct sqlite3_stmt;
struct sqlite3;
namespace wallet {
```
π rkrux approved a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753#pullrequestreview-3404583895)
crACK 78d4d36730d44de93690b43f900a9202517291f6
(https://github.com/bitcoin/bitcoin/pull/33753#pullrequestreview-3404583895)
crACK 78d4d36730d44de93690b43f900a9202517291f6
π¬ ismaelsadeeq commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473375153)
Yes, I think itβs a good idea to prevent multiple calls to waitNext if there is no explicit use case, or have interruptWait identify and cancel a targeted wait?
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473375153)
Yes, I think itβs a good idea to prevent multiple calls to waitNext if there is no explicit use case, or have interruptWait identify and cancel a targeted wait?
π¬ maflcko commented on pull request "test: Format strings in `test_runner`":
(https://github.com/bitcoin/bitcoin/pull/33753#issuecomment-3473395016)
lgtm ACK 78d4d36730d44de93690b43f900a9202517291f6
(https://github.com/bitcoin/bitcoin/pull/33753#issuecomment-3473395016)
lgtm ACK 78d4d36730d44de93690b43f900a9202517291f6
π¬ ryanofsky commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3473398583)
Thanks for the updated description. This all seems reasonable and is helpful. I do agree with Sjors though it could be good to move the information about next steps into #33389 which could become a tracking issue. Or you could close #33389 and make a new tracking issue (if things have changed since #33389 was opened). Also the current description ends abruptly with "It would be nice to also" so looks like some text is missing.
Other thoughts and suggestions:
- As long as caching is impleme
...
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3473398583)
Thanks for the updated description. This all seems reasonable and is helpful. I do agree with Sjors though it could be good to move the information about next steps into #33389 which could become a tracking issue. Or you could close #33389 and make a new tracking issue (if things have changed since #33389 was opened). Also the current description ends abruptly with "It would be nice to also" so looks like some text is missing.
Other thoughts and suggestions:
- As long as caching is impleme
...
π instagibbs approved a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3404624528)
reACK ed23b4537ae8a8a814738909fe9a84c548f2855d
`git range-diff master 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a ed23b4537ae8a8a814738909fe9a84c548f2855d`
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3404624528)
reACK ed23b4537ae8a8a814738909fe9a84c548f2855d
`git range-diff master 1ae9fd3f7a4fddc786e9921e7fc2af41ab96bf9a ed23b4537ae8a8a814738909fe9a84c548f2855d`
π¬ ryanofsky commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473445529)
> Yes, I think itβs a good idea to prevent multiple calls to waitNext if there is no explicit use case, or have interruptWait identify and cancel a targeted wait?
I think I like the current implementation more than either of these two ideas, because they both seem like they would bring unneeded complexity. It seems ok to allow multiple wait calls, since they should work perfectly well, even if there is not a very convenient way to cancel them. If it ever becomes important for interruptWait to
...
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473445529)
> Yes, I think itβs a good idea to prevent multiple calls to waitNext if there is no explicit use case, or have interruptWait identify and cancel a targeted wait?
I think I like the current implementation more than either of these two ideas, because they both seem like they would bring unneeded complexity. It seems ok to allow multiple wait calls, since they should work perfectly well, even if there is not a very convenient way to cancel them. If it ever becomes important for interruptWait to
...
π¬ Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473456047)
I successfully tested this PR against my Template Provider implementation, see https://github.com/Sjors/sv2-tp/pull/57.
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3473456047)
I successfully tested this PR against my Template Provider implementation, see https://github.com/Sjors/sv2-tp/pull/57.
π¬ A-Manning commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481717919)
Updated to make this a bit more succinct. `ptrdiff_t` used here because it is the signed equivalent to `size_t`, and is suitable for casting to `size_t` to compare against `headers.size()`.
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481717919)
Updated to make this a bit more succinct. `ptrdiff_t` used here because it is the signed equivalent to `size_t`, and is suitable for casting to `size_t` to compare against `headers.size()`.
π€ stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3395010361)
Comments on commits 8 (ffd4ae23) through 15 (0e973b20) and a couple of follow-ups.
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3395010361)
Comments on commits 8 (ffd4ae23) through 15 (0e973b20) and a couple of follow-ups.
π¬ stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474390216)
Seems unnecessary since `Block` is extending a public `Handle` (also `friend` statements in `BlockTreeEntry`). Seems only the ones currently extending non-public `UniqueHandle`s would need the `friend` statements. Any reason why extending `View` and `Handle` is done publicly but not for `UniqueHandle`?
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474390216)
Seems unnecessary since `Block` is extending a public `Handle` (also `friend` statements in `BlockTreeEntry`). Seems only the ones currently extending non-public `UniqueHandle`s would need the `friend` statements. Any reason why extending `View` and `Handle` is done publicly but not for `UniqueHandle`?
π¬ stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474403542)
Unused variable. Also line 661.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474403542)
Unused variable. Also line 661.
π¬ stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474982074)
Maybe not important but we could use `::ref()` to avoid the copy you [mentioned](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468790150)? (`CChainParams` copy ctor is called in `::create()` right now)
```cpp
btck_ChainParameters::ref(const_cast<CChainParams*>(CChainParams::Main().release()));
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474982074)
Maybe not important but we could use `::ref()` to avoid the copy you [mentioned](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468790150)? (`CChainParams` copy ctor is called in `::create()` right now)
```cpp
btck_ChainParameters::ref(const_cast<CChainParams*>(CChainParams::Main().release()));
```