📝 Sjors opened a pull request: "mining: add getTransactions(ByWitnessID) IPC methods"
(https://github.com/bitcoin/bitcoin/pull/34020)
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.
The spec doesn't specify how this is achieved, but one method is to call the `getrawtransaction` RPC on each transaction id listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server) (or a subset if the pool software maintains a cache). Using RPC is ineffici
...
(https://github.com/bitcoin/bitcoin/pull/34020)
For Stratum v2 custom job declaration to be bandwidth efficient, the pool can request[^0] only the transactions that it doesn't know about.
The spec doesn't specify how this is achieved, but one method is to call the `getrawtransaction` RPC on each transaction id listed in [DeclareMiningJob](https://stratumprotocol.org/specification/06-Job-Declaration-Protocol?query=DeclareMiningJob#644-declareminingjob-client-server) (or a subset if the pool software maintains a cache). Using RPC is ineffici
...
💬 Sjors commented on issue "rpc: getrawtransaction lookup by witness txid":
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3618231614)
See #34020 for the IPC approach.
(https://github.com/bitcoin/bitcoin/issues/34013#issuecomment-3618231614)
See #34020 for the IPC approach.
🤔 arejula27 reviewed a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3546080852)
conceptACK [fa76a6620012fb738639d8fd7ce17b185bfd376c]
This PR is valuable because it creates a smooth path toward adopting the new C++23 standard. Instead of requiring a full refactor to `std::expected` all at once, it allows the codebase to transition gradually, simply by replacing `utils::expected` with `std::expected` later on.
I also like the idea of introducing this error handling paradigm into the repository. `expected` is a clean way to handle errors and aligns with the modern progr
...
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3546080852)
conceptACK [fa76a6620012fb738639d8fd7ce17b185bfd376c]
This PR is valuable because it creates a smooth path toward adopting the new C++23 standard. Instead of requiring a full refactor to `std::expected` all at once, it allows the codebase to transition gradually, simply by replacing `utils::expected` with `std::expected` later on.
I also like the idea of introducing this error handling paradigm into the repository. `expected` is a clean way to handle errors and aligns with the modern progr
...
💬 arejula27 commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2593725731)
I think the current wording is a bit vague. It might be rewritten along the lines of:
```
//! It is intended for high-level functions that need to report error strings **directly**
//! to end users. Any lower-level functions that don't need this error-reporting...
```
However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:
>Result may also be a better choice than std::expected wh
...
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2593725731)
I think the current wording is a bit vague. It might be rewritten along the lines of:
```
//! It is intended for high-level functions that need to report error strings **directly**
//! to end users. Any lower-level functions that don't need this error-reporting...
```
However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:
>Result may also be a better choice than std::expected wh
...
💬 arejula27 commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2593700975)
I wouldn’t modify `src/net_processing.h` in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use std::optional for errors, for example, line 828 in `src/netbase.cpp`. Updating only these functions and not the others feels arbitrary and unrelated to the scope of this PR. I suggest moving these changes to a separate PR and making the update more exhaustive.
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2593700975)
I wouldn’t modify `src/net_processing.h` in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use std::optional for errors, for example, line 828 in `src/netbase.cpp`. Updating only these functions and not the others feels arbitrary and unrelated to the scope of this PR. I suggest moving these changes to a separate PR and making the update more exhaustive.
🤔 Crypt-iQ reviewed a pull request: "net: make m_nodes_mutex non-recursive"
(https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3546352523)
crACK 1a9274f391df0465c17b6b6664988af70910e39b
Needs a small rebase since #33956 added 2 annotations.
(https://github.com/bitcoin/bitcoin/pull/32394#pullrequestreview-3546352523)
crACK 1a9274f391df0465c17b6b6664988af70910e39b
Needs a small rebase since #33956 added 2 annotations.
💬 Crypt-iQ commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2593910296)
I was initially concerned that maybe `lNodesAnnouncingHeaderAndIDs.front()` could have been removed from in between releasing the lock and acquiring the lock again. It seems fine since if we do not find `lNodesAnnouncingHeaderAndIDs`, we don't need to set `m_bip152_highbandwidth_to` false and we still remove it from the list. There was some previous discussion about the locking [here](https://github.com/bitcoin/bitcoin/pull/22147#issuecomment-855904105).
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2593910296)
I was initially concerned that maybe `lNodesAnnouncingHeaderAndIDs.front()` could have been removed from in between releasing the lock and acquiring the lock again. It seems fine since if we do not find `lNodesAnnouncingHeaderAndIDs`, we don't need to set `m_bip152_highbandwidth_to` false and we still remove it from the list. There was some previous discussion about the locking [here](https://github.com/bitcoin/bitcoin/pull/22147#issuecomment-855904105).
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2594031239)
I like this. The only downside is that moving the functions from `Chainstate` to `TestChainstateManager` is less flexible if ever we wanted to have multiple chainstates.
I did a similar same approach also for blockmanager etc., so that now we only need to move a few things from `private` to `protected` in the production part of the code.
(https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2594031239)
I like this. The only downside is that moving the functions from `Chainstate` to `TestChainstateManager` is less flexible if ever we wanted to have multiple chainstates.
I did a similar same approach also for blockmanager etc., so that now we only need to move a few things from `private` to `protected` in the production part of the code.
💬 mzumsande commented on pull request "fuzz: Add fuzz target for block index tree and related validation events":
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3618658872)
Thanks for the reviews - with the most recent push I addressed https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2571742382 - will address the comments by @Crypt-iQ a bit later.
(https://github.com/bitcoin/bitcoin/pull/31533#issuecomment-3618658872)
Thanks for the reviews - with the most recent push I addressed https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2571742382 - will address the comments by @Crypt-iQ a bit later.
💬 billymcbip commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#discussion_r2594168890)
That's exactly right, but I think there was a mistake in the test. It should use `OP_1` for the second signature instead of `OP_0`, because `IsValidSignatureEncoding` is not run for empty signatures.
(https://github.com/bitcoin/bitcoin/pull/33973#discussion_r2594168890)
That's exactly right, but I think there was a mistake in the test. It should use `OP_1` for the second signature instead of `OP_0`, because `IsValidSignatureEncoding` is not run for empty signatures.
💬 billymcbip commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3618890756)
Updated `CHECKMULTISIG NOT` tests:
- Removed a comment referencing a test file that no longer exists.
- Improved test descriptions.
- Added negative cases to make the tests easier to understand.
- Changed invalid signature push from `OP_0` to `OP_1` because `IsValidSignatureEncoding` isn't called for empty sigs.
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3618890756)
Updated `CHECKMULTISIG NOT` tests:
- Removed a comment referencing a test file that no longer exists.
- Improved test descriptions.
- Added negative cases to make the tests easier to understand.
- Changed invalid signature push from `OP_0` to `OP_1` because `IsValidSignatureEncoding` isn't called for empty sigs.
💬 billymcbip commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3618909039)
On second thought, I think it's better to simply replace the `STRICTENC` flag with `DERSIG` when applicable instead of creating new tests. It makes the test file easier to parse.
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3618909039)
On second thought, I think it's better to simply replace the `STRICTENC` flag with `DERSIG` when applicable instead of creating new tests. It makes the test file easier to parse.
💬 fjahr commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3618988487)
Could you add a simple test for this?
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3618988487)
Could you add a simple test for this?
💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594622794)
> I wouldn’t modify `src/net_processing.h` in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use `std::optional` for errors, for example, line 828 in `src/netbase.cpp`.
No, it is not. This is a valid and intended use of `std::optional`:
```
std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
if (addr.has_value()) {
```
I'll leave as-is for now. Also, I'll keep touching the file, so that a valid and in
...
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594622794)
> I wouldn’t modify `src/net_processing.h` in this PR. Even though it’s a good showcase of the improvement, there are other functions that still use `std::optional` for errors, for example, line 828 in `src/netbase.cpp`.
No, it is not. This is a valid and intended use of `std::optional`:
```
std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
if (addr.has_value()) {
```
I'll leave as-is for now. Also, I'll keep touching the file, so that a valid and in
...
💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594623314)
> However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:
>
> > Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)
Thx, this is a good point. However, I think modifying the Result and Expected docs for ResultPtr is unrelated to this pull and can be done in the pull
...
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594623314)
> However, Result in our codebase isn’t limited to reporting errors to end users. As @ryanofsky noted in PR #25665, there are broader use cases. One strong example is:
>
> > Result may also be a better choice than std::expected when returning pointer values. (Followup #26022 adds a ResultPtr specialization that avoids double dereferencing.)
Thx, this is a good point. However, I think modifying the Result and Expected docs for ResultPtr is unrelated to this pull and can be done in the pull
...
💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594623940)
I know, and this was intentional. I can't imagine a real code snippet in this code-base that uses `.err` or `.error()`. The only use-case of Unexpected is to serve as a marker, which Expected constructor to pick internally. Am I missing something?
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594623940)
I know, and this was intentional. I can't imagine a real code snippet in this code-base that uses `.err` or `.error()`. The only use-case of Unexpected is to serve as a marker, which Expected constructor to pick internally. Am I missing something?
💬 Chand-ra commented on pull request "fuzz: Add a test case for `ParseByteUnits()`":
(https://github.com/bitcoin/bitcoin/pull/34017#issuecomment-3619725059)
> `ParseByteUnits` is not publicly exposed, i.e. it doesn't handle untrusted inputs, and I would not consider adding fuzz tests for this type of function as a priority.
Makes perfect sense. But I wonder why fuzz tests for these internal utilities (like `ToUpper()`, `ToLower()`, etc.) were introduced in the first place? Is it being thorough just for the sake for being thorough?
> As the in-repo fuzz tests are pretty saturated, it can be hard to spot valuable areas to improve (especially if
...
(https://github.com/bitcoin/bitcoin/pull/34017#issuecomment-3619725059)
> `ParseByteUnits` is not publicly exposed, i.e. it doesn't handle untrusted inputs, and I would not consider adding fuzz tests for this type of function as a priority.
Makes perfect sense. But I wonder why fuzz tests for these internal utilities (like `ToUpper()`, `ToLower()`, etc.) were introduced in the first place? Is it being thorough just for the sake for being thorough?
> As the in-repo fuzz tests are pretty saturated, it can be hard to spot valuable areas to improve (especially if
...
💬 Sjors commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594671525)
`FetchBlock()` is only used by the `getblockfrompeer` RPC (which I introduced in #20295), so it's nice and fairly isolated example to use.
Typically we gradually replace old code with more modern patterns while editing something in the same area, so the other examples will follow later.
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2594671525)
`FetchBlock()` is only used by the `getblockfrompeer` RPC (which I introduced in #20295), so it's nice and fairly isolated example to use.
Typically we gradually replace old code with more modern patterns while editing something in the same area, so the other examples will follow later.
👍 sedited approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3547326027)
ACK fa76a6620012fb738639d8fd7ce17b185bfd376c
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3547326027)
ACK fa76a6620012fb738639d8fd7ce17b185bfd376c
💬 Chand-ra commented on pull request "fuzz: wallet: add target for tx scanning":
(https://github.com/bitcoin/bitcoin/pull/32993#issuecomment-3619800359)
tACK [747d094](https://github.com/bitcoin/bitcoin/pull/32993/commits/747d0942413b26f0260126dfecf4f960a5e53a38)
The target runs without crashing.
(https://github.com/bitcoin/bitcoin/pull/32993#issuecomment-3619800359)
tACK [747d094](https://github.com/bitcoin/bitcoin/pull/32993/commits/747d0942413b26f0260126dfecf4f960a5e53a38)
The target runs without crashing.