💬 fjahr commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287143652)
#30207 isn't marked as fixing #26245 and also misses test coverage in `rpc_invalidateblock` so I think it's something different than the branch @mzumsande mentions?

💬 mzumsande commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287147028)
#30207 doesn't contain a fix for the `m_best_header` issue, it just cautions against using `m_best_header` for anything important (see first commit) and fixes some other related bugs.

💬 furszy commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287173535)
Ok, just remembered it. #30207 shares part of the fix branch, and thats why I moved there first.
@mzumsande, guess that your upcoming PR will continue containing the same shared commit "validation: mark blocks between m_best_header and invalid block as BLOCK_FAILED_CHILD" ?

So in any case, we could move forward with both PRs #30207 and the new one now that there is some review power :).

👍 willcl-ark approved a pull request: "validation: assumeutxo params mainnet"
(https://github.com/bitcoin/bitcoin/pull/28553#pullrequestreview-2236651580)
tACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410

Checked that the snapshot was created and had the same `sha256sum`

<details>
<summary>Details</summary>

```log
$ ./contrib/devtools/utxo_snapshot.sh 840000 snapshot.dat ./src/bitcoin-cli
Do you want to disable network activity (setnetworkactive false) before running invalidateblock? (Y/n): y
Disabling network activity
false
Rewinding chain back to height 840000 (by invalidating 00000000000000000001b48a75d5a3077913f3f441eb7e08c13c43f768
...

💬 ryanofsky commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2287225308)
> I think for most functions here it could be feasible to have more concise error codes without too much effort, but I feel like I have to detach from this a bit before being able to come up with an alternative.

Thanks, I think I'd need to look at this more to give concrete suggestions, but I'd hope most functions would just return a simple success or failure status, with a descriptive error message in the case of failure. When functions need to return more complicated information or can fail
...

💬 mjdietzx commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2287277959)
reACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501

🤔 tdb3 reviewed a pull request: "remove repeated word in note"
(https://github.com/bitcoin/bitcoin/pull/30651#pullrequestreview-2236775946)
NACK
Thanks for the interest.
Typically, these types of basic changes are saved for when more impactful changes are made.
Please see https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#refactoring

💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1716082861)
Ah, missed one, fixed.

💬 justinvforvendetta commented on pull request "remove repeated word in note":
(https://github.com/bitcoin/bitcoin/pull/30651#issuecomment-2287375699)
maybe you have a label for such?

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716215869)
Agree, it turned out it could be worked around by switching to `BOOST_CHECK_EQUAL_COLLECTIONS` in **key_tests.cpp**.

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716226929)
This is in response to https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1689162345

To summarize - the concern is not stack memory being used inside `ArrayFromBytes` itself, but rather that the `std::array` returned contains an inner C-array (https://github.com/gcc-mirror/gcc/blob/61715e9340ab8941d40d62158fe437e9dbe3e068/libstdc%2B%2B-v3/include/std/array) that is not allocated from the heap and so will bump the stack pointer in the calling function by a fair bit for long hex strings.
...

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval ArrayFromHex":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1716229145)
I was running into an issue with `CScript` (inheriting `prevector`) and `std::array`s, similar to here: https://gitlab.freedesktop.org/pipewire/media-session/-/issues/4

Reverted this change in the latest push as I'm no longer modifying `CScript`.

💬 maflcko commented on pull request "feat(build): improve dependency error reporting in autogen.sh":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2288031311)
Tend toward NACK. No need to apply style fixes to a file that will be deleted in 10 days (or whenever the branch-off happens and the cmake pull is reviewed sufficiently)

💬 maflcko commented on pull request "doc: Deduplicate list of possible chain strings in RPC help texts":
(https://github.com/bitcoin/bitcoin/pull/30648#issuecomment-2288040823)
lgtm ACK 9b297555207b4ea54bc0051f09c7084797aa9def

💬 maflcko commented on pull request "remove repeated word in note":
(https://github.com/bitcoin/bitcoin/pull/30651#issuecomment-2288041109)
ACK 3f05a1068d10ffe0f2859cd20c5fc9bc8efa1c70

please add the `fuzz: ` prefix to the pull title.

💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716446494)
I don't like the suggestion, because it makes it harder to change `GetNetworkForMagic` to accept a span in the future. Generally, when a function accepts a read-only non-lifetimebound view of a byte blob, a span can be used. `std::span` supports fixed size and dynamic size, so the prior workarounds to force a fixed-size array no longer apply and it seems odd to rely on them in new code.

💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1716448092)
Leaving as-is for now to not invalidate review. I'll follow-up with iwyu on all files, I guess.

💬 maflcko commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2288076458)
> `nproc` is linux only

I don't think this is true. I don't have a mac, but I fail to see why installing it won't work. Also, a bash alias similar to `alias nproc="sysctl -n hw.logicalcpu"` or `alias nproc="getconf _NPROCESSORS_ONLN"` should work as well.

No objection to changing this, but I think the claim that `nproc` is linux-only may not be true.

👍 MarnixCroes approved a pull request: "doc: Deduplicate list of possible chain strings in RPC help texts"
(https://github.com/bitcoin/bitcoin/pull/30648#pullrequestreview-2237465538)
ACK 9b297555207b4ea54bc0051f09c7084797aa9def

💬 maflcko commented on issue "contrib: Automation for Bitcoin Full Node Deployment":
(https://github.com/bitcoin/bitcoin/issues/30638#issuecomment-2288148372)
> Regarding Dockerfile, I believe it can be considered part of Bitcoin Core Software, as a Bitcoin Core user I need it and I can see through the discussions that many need it, could someone enlighten me of what is needed to start accepting Dockerfile as part of Bitcoin Core Software? What are your requirements?

No one is holding anyone back from opening a pull request to add a container engine imagefile. Not sure how much actual interest is there, given that no one has done so far, or the pre
...