📝 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.

💬 andrewtoth commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596422778)
We can at least change the declaration to not be an optional:
```
std::pair<size_t, size_t> block_part;
```

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2596452456)
WDYT about https://github.com/bitcoin/bitcoin/compare/9efccb0d3e84d83e0ec29a4b18b72019ffc3d5c3..262f4dfe691b12171a31c9219ba33f1fe55f67a7?

💬 sedited commented on pull request "validation: Remove min_pow_checked arg in ProcessNewBlockHeaders":
(https://github.com/bitcoin/bitcoin/pull/34022#discussion_r2596516751)
See https://github.com/sedited/bitcoin/tree/rmHeaderLowWork :)

The reason we are using it in net_processing is the following: We might receive an unrequested block whose block header we previously have not processed and which forks off at a low-work point. A peer sending as such a block is not punished, but the message is essentially just ignored. We could just check for that condition in net_processing and return early, but then we are also skipping a bunch of validation work, that might cat
...

👍 sedited approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3549497745)
Re-ACK faa23738fc2576e412edb04a4004fab537a3098e

📝 codeabysss opened a pull request: "[p2p] Saturate LocalServiceInfo::nScore to prevent overflow"
(https://github.com/bitcoin/bitcoin/pull/34028)
This prevents signed integer overflow in `LocalServiceInfo::nScore` by saturating at `std::numeric_limits<int>::max()` in `SeenLocal()`.

The `nScore` field could overflow from `INT_MAX` to `INT_MIN` when incremented during version handshakes, causing undefined behavior.

I implemented saturation in `SeenLocal()` to cap `nScore` at `std::numeric_limits<int>::max()` instead of allowing overflow. This prevents undefined behavior while maintaining memory efficiency - after 2 billion connections
...

💬 ajtowns commented on pull request "test: p2p: check that peer's announced starting height is remembered":
(https://github.com/bitcoin/bitcoin/pull/33990#issuecomment-3623426022)
> Yeah, I also wonder what the use-case here is or was.

Looks like it was introduced in v0.2.9 where it was used as a heuristic for determining if we're in IBD (due to being more than 2k blocks behind where our peers were), The check was removed in #20624, which also has some more context.

💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#issuecomment-3623680270)
Now a proper repository for the Python code: https://github.com/sipa/pyclusterlin

💬 ryanofsky commented on pull request "mining: add getTransactions(ByWitnessID) IPC methods":
(https://github.com/bitcoin/bitcoin/pull/34020#issuecomment-3624001654)
>figure out [Treat std::shared_ptr nullptr as empty Data bitcoin-core/libmultiprocess#235](https://github.com/bitcoin-core/libmultiprocess/issues/235) (this PR is draft pending that, subtree linter failure is expected)

This turned out to be an interesting problem, because in general it's not safe to treat empty `Data` fields the same as unset values. For example with `std::optional<std::string>` or `std::string*` we would want to interpret empty `Data` fields as empty strings not null values.
...

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2596842637)
Do we really need to pass the dirty count here? We always iterate through the entire cursor, so we can just set `m_dirty_count = 0;` after a `Sync` or `Flush` (we already do for `Flush`).

📝 Osptium-Invincible opened a pull request: "added waring lmao"
(https://github.com/bitcoin/bitcoin/pull/34029)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2597340445)
Hi @rkrux there is an issue [CI / Windows, ucrt, test cross-built (pull_request)](https://github.com/bitcoin/bitcoin/actions/runs/19963803010/job/57251640476?pr=31668)Failing after 18m but in Linux it passed do you have any idea how to fix this.

💬 Chand-ra commented on pull request "refactor: C++20 operators":
(https://github.com/bitcoin/bitcoin/pull/33771#issuecomment-3625675526)
tACK [`48840bf`](https://github.com/bitcoin/bitcoin/commit/48840bfc2d7beeac0ddf56a3c26b243156ec8936). Built the PR and ran unit tests; everything passes.

Verified that all instances of `operator!=` have been removed and `operator<=>` replaces combined implementations of `<`, `<=`, `>`, and `>=`. (Note: PR #33772 and not this one gets rid of `operator<` in [`src/prevector.h`](https://github.com/bitcoin/bitcoin/blob/89dc82295ebd8a959fb5f6e7a116b6ce9e44e340/src/prevector.h#L458).)

💬 Sjors commented on pull request "mining: add getTransactions(ByWitnessID) IPC methods":
(https://github.com/bitcoin/bitcoin/pull/34020#issuecomment-3625705058)
Thanks, I swapped in your commit (subtree linter is still expected to fail).

> So your workaround of interpreting empty Data values as null CTransactionRef is probably the best approach for now.

One reason I have some confidence in this approach is that the Python functional tests are able to read the result. Although it interprets the empty fields as `b''` rather than `None`, that shouldn't be an issue for client developers.

👍 hodlinator approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3550883332)
ACK faa23738fc2576e412edb04a4004fab537a3098e

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597671961)
(Tried old-school `(void)connection_client.release();` and `[[maybe_unused]] (void)connection_client.release();` to avoid the noisy scope but they don't pass clang-tidy).

💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2597679959)
The above patch + `Assume()` would be nice but I won't insist if you prefer not to, in this case.