👍 Sjors approved a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521#pullrequestreview-2857047398)
ACK 29e8fe71440a7e27ee89527a49753be615f205c7

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099855247)
29e8fe71440a7e27ee89527a49753be615f205c7: note to other reviewers, yes `generate` checks for errors, as you can see if you duplicate `nonstd_tx.serialize().hex()`

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099858669)
29e8fe71440a7e27ee89527a49753be615f205c7: `MAX_TX_LEGACY_SIGOPS` would be consistent with the c++ code

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099863081)
a139a538c298621674ba622e63971704cbb7ceff: what's this magic `424242` value?

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099872077)
a139a538c298621674ba622e63971704cbb7ceff: could insert a segwit spend to show that it doesn't count.

💬 Sjors commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2099865784)
a139a538c298621674ba622e63971704cbb7ceff: it seems more clear if you define it as an integer instead of auto.

💬 Sjors commented on issue "Dropping unused legacy wallet code":
(https://github.com/bitcoin-core/gui/issues/873#issuecomment-2897367471)
@achow101 also checked that there doesn't appear to be more lingering stuff: https://github.com/bitcoin/bitcoin/pull/32459#issuecomment-2887600076

⚠️ fanquake opened an issue: "test: `tool_wallet.py` references no-longer used CI"
(https://github.com/bitcoin/bitcoin/issues/32576)
We no longer use Appveyor, so any code that existed to support it, should be dropped:

https://github.com/bitcoin/bitcoin/blob/54e406a3055ad0454a576f14b51def34ab8e9209/test/functional/tool_wallet.py#L160

https://github.com/bitcoin/bitcoin/blob/54e406a3055ad0454a576f14b51def34ab8e9209/test/functional/tool_wallet.py#L171

💬 fanquake commented on pull request "build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2897381770)
I'll come back to this once we land the changes upstream.

✅ fanquake closed a pull request: "build: remove need to test for endianness"
(https://github.com/bitcoin/bitcoin/pull/29852)

🤔 Sjors reviewed a pull request: "doc: Remove stale sections in dev notes"
(https://github.com/bitcoin/bitcoin/pull/32572#pullrequestreview-2857125028)
I see the benefit of not duplicating linter code in developer notes. However it's quite annoying to find out about linter failures only after pushing.

The developer notes could point to the incantation to run all the linters in a container.

Otherwise looks good.

💬 Sjors commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2099905797)
fa220ec193436fd1e62e29a8f3525bf26fa3c209: this is still useful, even if often ignored :-)

💬 Sjors commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2099904688)
`.gitignore` is indeed a great place to document this, as it's the file anyone will attempt to edit when their IDE leaves an artefact behind.

💬 maflcko commented on pull request "ci: Avoid && dropping errors":
(https://github.com/bitcoin/bitcoin/pull/32573#issuecomment-2897439704)
> Could this approach be forced by a linter?

I don't know how, because `( a && b )` is used and is fine. My preference would be to just stop using bash, but writing this one in Python or Rust will bloat the code. :man_shrugging:

💬 stickies-v commented on pull request "refactor: Split multithreaded case out of CheckInputScripts":
(https://github.com/bitcoin/bitcoin/pull/32575#issuecomment-2897442292)
Concept ACK

💬 maflcko commented on pull request "doc: Remove stale sections in dev notes":
(https://github.com/bitcoin/bitcoin/pull/32572#discussion_r2099937657)
> often ignored :-)

I don't see any bash scripts. Apart from pre-existing scripts (ci and contrib stuff).



> this is still useful

It should probably say Rust, though? :sweat_smile:

💬 rkrux commented on issue "listunspent, fundrawtransaction, getwalletinfo locks wallet for any other operation":
(https://github.com/bitcoin/bitcoin/issues/27002#issuecomment-2897455395)
> So far, we've been focusing on the low hanging fruit of reducing the runtime of operations that are known to take a long time.

I am in agreement with this approach. Reducing the RPCs' latencies by using some additional memory seems like a reasonable tradeoff. Lesser latency would lead to a lesser amount of time that the lock is in place.

💬 laanwj commented on pull request "subprocess: Backport upstream changes":
(https://github.com/bitcoin/bitcoin/pull/32567#issuecomment-2897470684)
The change is important for #32566 because the functional test was failing for the wallet notification, due to the extra space ending up in the output of `echo`.

i don't particularly mind how it ends up in the repository.

💬 laanwj commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2099962190)
Wasn't sure about this. One reason for setting `ENABLE_SUBPROCESS=OFF` in the first place would be that the operating system doesn't support subprocesses. As subprocess is a header-only library, including the `subprocess.hpp` header would mean use of `fork` `execv` `CreateProcessW` etc. So it could break the compile.

💬 laanwj commented on pull request "Use subprocess library for notifications":
(https://github.com/bitcoin/bitcoin/pull/32566#discussion_r2099964540)
If we don't care about supporting such operating systems (only iOS comes to mind), it would imo be preferable to remove the option entirely.