💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851603)
Added a comment

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851636)
Reworded the comment.

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851676)
Added a comment

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851698)
Done

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851723)
Reworded the comment.

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1844851735)
Reworded the comment.

💬 achow101 commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#issuecomment-2480325630)
> There is a bug in the second loop. Before adding the P2WSH program to the spks set, it is necessary to verify that the underlying witness script does not contain any uncompressed keys, as these are prohibited under segwit rules.

I've added a test to `wallet_migration.py` so that it will persist once legacy wallets are removed.

💬 JeremyRand commented on issue "guix: Linux and macOS builds are not cross-arch reproducible with powerpc64le build arch":
(https://github.com/bitcoin/bitcoin/issues/31207#issuecomment-2480341752)
> > I would be happy to upload my binaries
>
> Yes, please do. i'm happy to try to help find the difference but i don't have access to a ppc64 system.

I've uploaded my non-debug binaries here:

https://ynqrnzvuotdgvqj2g73f5tyj4rcvwutdemu23jgadawihbkc3lxkbwid.onion/bitcoin-core/28.0-guix-binaries-built-on-powerpc64le/

If the debug variants would be helpful, I don't mind uploading them too. I don't have any plans to delete these files or decommission this server, but it's personal hobby
...

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1844883670)
I meant removing only "or other methods".

💬 Maxzy000 commented on issue "Tracepoint Interface Tracking Issue":
(https://github.com/bitcoin/bitcoin/issues/31274#issuecomment-2480348740)
Let me guide you on this

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-2480372333)
Even with just 2 worker threads, there is significant (~30%) speed improvement for syncing recent blocks.
https://bitcoin-dev-tools.github.io/benchcoin/results/pr-19/11865650166/index.html

💬 maflcko commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480473723)
> git checkout v28.0
> ubuntu 18.04
> cmake version 3.10.2 gcc version 10.3.0 g++ version 10.3.0

To learn more about the minimum required dependencies, please refer to https://github.com/bitcoin/bitcoin/blob/28.x/doc/dependencies.md

My recommendation would be to try to dist-upgrade your Ubuntu, as Ubuntu 18.04 hit end of standard support more than two years ago, according to https://wiki.ubuntu.com/ReleaseTeam

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844329517)
nit in cca0ddbaf4dde7c9b8b50f5f1fd29cefdcf301ea: Same here. I don't see the reason to have a different log message on `-par=1`, compared to `-par=2`.

Not that it matters, because this code is deleted anyway in a future commit. However, my recommendation would be to drop this 1-line hunk, so that no inconsistency is introduced and code churn in deleted lines is kept to a minimum.

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844296431)
nit in 73417066867ea7920647054eb0752e0dd3100de7: This is unrelated to making the error messages uniform, but I think it would be nice to use the same log level as well. Otherwise it seems a bit odd that `-par=1` will print a different log level than `-par=2`. (Personally I think that `LogInfo` is probably enough)

Edit: Doesn't matter, as this is deleted anyway in a later commit.

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844940865)
style nit in the last commit: For new code my recommendation would be to use `LogInfo`, instead of the deprecated alias, which does exactly the same.

<!--
However, I am somewhat thinking that all non-bench logging in this function can be deleted, as all callers will redundantly log the state, except for one. So it may be clearer, less code, and less log-spam to ensure every validation error is logged exactly once (in the caller).

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844943672)
nit in 011d196c6fff4bbcd2bd03998c64b704091d0aff: Reads a bit odd to invert the return value in this case, but I understand that keeping it return error isn't trivially possible and doesn't matter anyway. Maybe add a comment `// return value does not matter, because m_request_stop is only set in the destructor` ?

🤔 maflcko reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2439353625)
Just some non-blocking nits. Feel free to ignore.

review ACK cc6d3ea623933cc7f0db0daaabcbdda86272c8f7 🐱

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNL
...

💬 Sjors commented on issue "Proposed Timeline for Legacy Wallet and BDB removal":
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-2480556327)
I wrote two years ago:

> However, not too soon.

Ok, now is fine :-)

It seems #30328 and #31248 is next on the review list and then actual removal in #28710?

💬 hebasto commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480631513)
> cmake version 3.10.2

This CMake version does not understand the `-S` and `-B` command line options, which were introduced in the version 3.13.

@techy2

You can download CMake 3.22 or newer, which is required for the main build system, from https://cmake.org/download/.

💬 hebasto commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2480658902)
Concept ACK.