🤔 marcofleon reviewed a pull request: "fuzz: Add a test case for `ParseByteUnits()`"
(https://github.com/bitcoin/bitcoin/pull/34017#pullrequestreview-3545708127)
crACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8
Ran it for a bit as a sanity check, seems fine.
(https://github.com/bitcoin/bitcoin/pull/34017#pullrequestreview-3545708127)
crACK 57b888ce0ebdeb34d866fd1511052fd740cc5ab8
Ran it for a bit as a sanity check, seems fine.
💬 willcl-ark commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3617838716)
Thanks @ryanofsky, I took your clarifying changes in 2cf352fd8e6a77003e38d954b6c879b20d4b960a
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3617838716)
Thanks @ryanofsky, I took your clarifying changes in 2cf352fd8e6a77003e38d954b6c879b20d4b960a
💬 ajtowns commented on issue "RFC: randomize over netgroups in outbound peer selection":
(https://github.com/bitcoin/bitcoin/issues/34019#issuecomment-3617990387)
I seem to have 7204 ipv4 nodes in my tried table with a timestamp more recent than 90 days ago, split across 3509 /16s. There are 6 /16s with between 100 and 200 tried entries, and another 23 /16s with more than 20 tried entries. At the other end of the scale, there are 2578 /16s with only one node in my tried table, 561 with two nodes, 172 with three, 58 with four, 28 with 5 and 26 with 6.
The network is able to accept 115 inbound connections per node by default (max connections = 125, minus 1
...
(https://github.com/bitcoin/bitcoin/issues/34019#issuecomment-3617990387)
I seem to have 7204 ipv4 nodes in my tried table with a timestamp more recent than 90 days ago, split across 3509 /16s. There are 6 /16s with between 100 and 200 tried entries, and another 23 /16s with more than 20 tried entries. At the other end of the scale, there are 2578 /16s with only one node in my tried table, 561 with two nodes, 172 with three, 58 with four, 28 with 5 and 26 with 6.
The network is able to accept 115 inbound connections per node by default (max connections = 125, minus 1
...
💬 ajtowns commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-3618027049)
ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-3618027049)
ACK fa4395dffd432b999002dfd24eb6f8d7384fbcbe
📝 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?