💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2596258697)
In modern languages like Rust, yes. But not in C++, because there is no order of evaluation of the function arguments.

So `make_unique(client->m_rpc, client.release())` will lead to a nullptr deref (segfault).

I could rewrite this to use `new ProxyClient{client->m_rpc, client.release()}`, because for braced-init, the order is defined, but I am not sure if this is better, because it violates https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-unique.html

💬 maflcko commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2596259276)
> I would like you to consider adding **directly** as I mentioned, as you already did some small text corrections on `results.h`

Not sure. I am not touching this line, so I'll leave as-is for now.

💬 maflcko commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3621967700)
`LogPrintLevel` isn't really documented in the dev notes and the unit test name `logging_LogPrintMacrosDeprecated` seems to indicate it is deprecated. This change here seems to put a use-case to it, so it could make sense to document and test it? Though, I wonder instead of using the `LogPrintLevel` macro, it could make more sense to just add a new `LogInfo` macro that mirrors the `LogDebug` signature? I can't really see a use-case to have high-volume warnings or errors, so that they run into ra
...

💬 ajtowns commented on pull request "log: don't rate-limit "new peer" with -debug=net":
(https://github.com/bitcoin/bitcoin/pull/34008#discussion_r2596296831)
Yeah. If it's not obvious, the whole thing would look something like:

```c++
auto new_peer_info = [&]() {
// lambda so that this info is only calculated if needed
const auto mapped_as{m_connman.GetMappedAS(pfrom.addr)};
return strprintf("transport: %s version: %d, blocks=%d, peer=%d%s%s\n",
TransportTypeAsString(pfrom.m_transport->GetInfo().transport_type),
pfrom.nVersion.load(), peer->m_starting
...

💬 Ataraxia009 commented on pull request "log: exempt all category-specific logs from ratelimiting when running with debug":
(https://github.com/bitcoin/bitcoin/pull/34018#issuecomment-3622056086)
> > if `-debug` is used with this implementation, it would stop rate-limiting of all category logs.
>
>
>
> Debug logs are (by design) much higher frequency than info/warning/error level logs. They have never been ratelimited, because for debugging purposes it's important to see what's happening without limitation. When running with `-debug`, the overwhelming majority of logs produced are debug logs. This PR doesn't change anything about any of that.
>
>
>
> Info logs are typically much hi
...

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332437)
Fixed in 7802c1f9ca09b5669b8400e96baeadc238fa9df2

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332557)
Fixed in 7802c1f9ca09b5669b8400e96baeadc238fa9df2

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596332829)
Used `util::Expected` from fa114be27b17ed32c1d9a7106f313a0df8755fa2

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596333260)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596333791)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596334064)
Fixed in 71c7b747091fce045dbf25e4e0cfc0e6a4ad6435

💬 aandrews26 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3622209446)
> Are you still working on this?

Yes, I will address all comments in a patch in the next few days.

💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#issuecomment-3622210884)
> Are you still working on this?

Yes, I will produce an additional patch addressing all comments in the coming days.

✅ billymcbip closed a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961)

✅ billymcbip closed a pull request: "test: Improve STRICTENC/DERSIG unit tests in script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973)

💬 darosior commented on pull request "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript":
(https://github.com/bitcoin/bitcoin/pull/33961#issuecomment-3622335233)
Force-push gone wrong @billymcbip? Or is this intentional?

📝 billymcbip reopened a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961)
We currently have two callsites for `SCRIPT_ERR_PUBKEYTYPE`:
- A pre-tapscript policy error behind the `SCRIPT_VERIFY_STRICTENC` flag: https://github.com/bitcoin/bitcoin/blob/4de26b111f4d9b116d0d4930f8bbd69889b3cf75/src/script/interpreter.cpp#L220
- A [consensus error](https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki?plain=1#L93) in Tapscript: https://github.com/bitcoin/bitcoin/blob/4de26b111f4d9b116d0d4930f8bbd69889b3cf75/src/script/interpreter.cpp#L368

It would be good for r
...

💬 billymcbip commented on pull request "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript":
(https://github.com/bitcoin/bitcoin/pull/33961#issuecomment-3622350315)
@darosior that was unintentional, sorry! I still intend to ship 9d5021a.

📝 billymcbip reopened a pull request: "test: Improve STRICTENC/DERSIG unit tests in script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973)
Use the `DERSIG` flag instead of the `STRICTENC` flag for signature encoding tests. `STRICTENC` rules are a superset of `DERSIG` rules, and only `DERSIG` is a consensus flag. Also improve test descriptions and add two error cases to the `CHECKMULTISIG NOT` section to make it easier to understand how these tests work.

Tests pass on `6224b272ac291afd3af89f91e1deaccf233718bf`:
```
cmake -B build -DENABLE_WALLET=OFF
cmake --build build --parallel 8
ctest --test-dir build --parallel 8
```
``
...

💬 billymcbip commented on pull request "test: Improve STRICTENC/DERSIG unit tests in script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#issuecomment-3622371459)
Closed by accident. I still intend to ship this change.