💬 fjahr commented on pull request "rpc: Set best header in reconsiderblock":
(https://github.com/bitcoin/bitcoin/pull/30652#issuecomment-2287095138)
@mzumsande thanks
(https://github.com/bitcoin/bitcoin/pull/30652#issuecomment-2287095138)
@mzumsande thanks
💬 fjahr commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287099485)
Concept ACK
@furszy Are you still working on this? I need a fix for this to do better testing in #29553.
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287099485)
Concept ACK
@furszy Are you still working on this? I need a fix for this to do better testing in #29553.
💬 mzumsande commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287106023)
I had a branch with a different solution that involves iterating over the entire block index (which is not super-efficient, but if it only is necessary in certain cases such as invalidateblock it may be ok?!): https://github.com/mzumsande/bitcoin/tree/202404_invalidblock
I think I'll clean that branch up in the next days and PR it as an alternative.
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287106023)
I had a branch with a different solution that involves iterating over the entire block index (which is not super-efficient, but if it only is necessary in certain cases such as invalidateblock it may be ok?!): https://github.com/mzumsande/bitcoin/tree/202404_invalidblock
I think I'll clean that branch up in the next days and PR it as an alternative.
💬 fjahr commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287110142)
> I think I'll clean that branch up in the next days and PR it as an alternative.
That would be great, thanks!
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287110142)
> I think I'll clean that branch up in the next days and PR it as an alternative.
That would be great, thanks!
🤔 hebasto reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2236548045)
Almost ACK fd087d4667b94051d00617189dcecc4f88d849ce.
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2236548045)
Almost ACK fd087d4667b94051d00617189dcecc4f88d849ce.
💬 hebasto commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1715909769)
> Fixed during rebase.
It still needs to be fixed.
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1715909769)
> Fixed during rebase.
It still needs to be fixed.
💬 furszy commented on pull request "bugfix: update chainman best_header after block reconsideration":
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287134402)
IIRC, we have #30207 containing @mzumsande's fix branch and my intention was to leave this open while was reviewing the other one but.. it seems I never pushed my review there.
(https://github.com/bitcoin/bitcoin/pull/29913#issuecomment-2287134402)
IIRC, we have #30207 containing @mzumsande's fix branch and my intention was to leave this open while was reviewing the other one but.. it seems I never pushed my review there.
💬 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?
(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.
(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 :).
(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
...
(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
...
(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
(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
(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.
(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?
(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**.
(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.
...
(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`.
(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)
(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)