📝 fanquake converted_to_draft a pull request: "lint/contrib/doc updates"
(https://github.com/bitcoin/bitcoin/pull/30084)

💬 fanquake commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2189305171)
Moved to draft for now, given it's not clear what the status of this is.

📝 fanquake converted_to_draft a pull request: "Enhanced error messages for invalid network prefix during address parsing."
(https://github.com/bitcoin/bitcoin/pull/27260)
### **Issue:**

Simply `DecodeDestination` does not handle errors where the user inputs a valid address for the wrong network. _e.x. testnet while running client on mainnet_

The current error `not a valid Bech32 or Base58 encoding` for a valid address on a different network is entirely incorrect. This is because of the is_bech32 variable used at the core of `DecodeDestination` logic only checks that the prefix matches. If it doesn't it just starts running everything as `DecodeBase58Check
...

💬 fanquake commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-2189309433)
Moved to draft for now, given at least one outstanding review comment and needs a rebase.

✅ fanquake closed a pull request: "doc: Add link to ccache docs for more description"
(https://github.com/bitcoin/bitcoin/pull/29604)

💬 fanquake commented on pull request "doc: Add link to ccache docs for more description":
(https://github.com/bitcoin/bitcoin/pull/29604#issuecomment-2189316076)
Thanks, however I think we'll leave this as-is.

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1653094080)
It was a "change all occurrences" accident. `version_os` was local to `parse_version_string`, so I logically assumed `version_rc` would be too. So I blame the original variable names for this goof.
I didn't see until after I had force pushed.

Shall I restrict the `version_rc` to `rc` rename to only within the `parse_version_string` function? Or not worth changing back?

📝 fanquake opened a pull request: "depends: update doc in Qt pwd patch"
(https://github.com/bitcoin/bitcoin/pull/30336)
Now that upstream has gotten around to fixing this. We don't need any more of the patch, and it likely wont apply to our version of Qt in any case. See: https://github.com/qt/qtbase/commit/3388de698bfb9bbc456c08f03e83bf3e749df35c.

📝 nnsW3 opened a pull request: "Docs improvements"
(https://github.com/bitcoin/bitcoin/pull/30337)
Rectify typographical inaccuracies

This PR addresses several typographical errors across various files in the project. The changes improve readability and maintain the professional standard of the documentation and code comments.

Justification
Typographical errors, while minor, can detract from the overall quality of the project. Correcting these errors ensures clarity and professionalism, making the project more accessible and understandable for current and future contributors.

💬 TheCharlatan commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2189380466)
Guix build (aarch64):
```
544f33855573c49ff642be39c54603c9afbf64076e6575cb19c5a64fea1522e4 guix-build-81d4dc8e8739/output/aarch64-linux-gnu/SHA256SUMS.part
a8a3c29fe1a2a21f677cfbd8fadb1d65f5901b6f133531f6fce736f1e1e163e5 guix-build-81d4dc8e8739/output/aarch64-linux-gnu/bitcoin-81d4dc8e8739-aarch64-linux-gnu-debug.tar.gz
d059d19cb255b3f555eb2be6b89e0d000a9476278aa783efdc1ba0c3faac6236 guix-build-81d4dc8e8739/output/aarch64-linux-gnu/bitcoin-81d4dc8e8739-aarch64-linux-gnu.tar.gz
1ab4dac0ea
...

💬 TheCharlatan commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#discussion_r1653136723)
That's fine too, it is not used anyway.

💬 stickies-v commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1653151031)
I'd prefer limiting it to `parse_version_string` and will quickly re-ack if you force push

💬 jonatack commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2189405730)
Hi @nnsW3, several of these proposed changes are incorrect. I suggest reading through https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md and https://jonatack.github.io/articles/how-to-contribute-pull-requests-to-bitcoin-core for an idea of contributions that would be useful to this project and valued. Cheers.

💬 BenWestgate commented on pull request "contrib: Fixup verify-binaries OS platform parsing":
(https://github.com/bitcoin/bitcoin/pull/30147#discussion_r1653163359)
Done in https://github.com/bitcoin/bitcoin/compare/4a85b154d0874f7bfa848ae8d7ab341eea754607..3ab25201909bece9066ac6191670bcee09791d54

👍 stickies-v approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2139226506)
re-ACK 3ab25201909bece9066ac6191670bcee09791d54

💬 ryanofsky commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2189439362)
> Maintainer note: `SKIP_BRANCH_PUSH=true` must be set in Cirrus for `bitcoin-core/gui` before merging this. See `https://cirrus-ci.com/github/bitcoin-core/gui` -> Settings.

I just now went to that URL and set this. There were not any other environment variables set.

I wonder if there is a good place to document this configuration, but probably it's not too important as the main effect is just to avoiding wastefully running CI twice on pushes to master (at least according to the original P
...

💬 Sjors commented on pull request "util/system: Close non-std fds when execing slave processes":
(https://github.com/bitcoin/bitcoin/pull/22417#issuecomment-2189525056)
> What is the status of this now that Boost Process is removed

For historical reference: this patch is no longer relevant, since #28981 replaced boost-process with cpp-subprocess.

@luke-jr does `cpp-subprocess` do anything similar that needs addressing? https://github.com/bitcoin/bitcoin/blob/master/src/util/subprocess.h

💬 Sjors commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2189534918)
@jlopp thanks for the testing and your article.

I assume "to reach a given block height with a 28 GB dbcache size" you did _not_ run with this patch? I'd be curious to see the result.

And if you feel like battle testing this further, you could make a custom signet, produce a similar UTXO set to mainnet (maybe #24952 can help), increase it to a few hundred gigabyte and then see if and where there's a value `-dbcache` where performance peaks and goes down.

📝 theuni opened a pull request: "RFC: Instanced logs"
(https://github.com/bitcoin/bitcoin/pull/30338)
PR'ing this early as an RFC because it's a lot of work that will require constant rebasing. This is the first logical step and is mostly mechanical.

This PR adds an "instance" param to each log function and callsite. As is, there should be no functional change here.

The intention is to allow for a specific instance to be passed in each time we use the logger, rather than the implicit global as we have it now. Eventually (one subsystem/chunk at a time), this will allow us to instantiate a p
...

💬 theuni commented on pull request "RFC: Instanced logs":
(https://github.com/bitcoin/bitcoin/pull/30338#issuecomment-2189606984)
Ping @TheCharlatan @ajtowns @maflcko for Concept (N)ACKs