🤔 DCMTOKEN reviewed a pull request: "rpc: Reword SighashFromStr error message"
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2256150820)
Please approve my request for changes
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2256150820)
Please approve my request for changes
🤔 DCMTOKEN reviewed a pull request: "rpc: Reword SighashFromStr error message"
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2256157906)
Please approve changes so I can run a test and see if there's any issues
(https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2256157906)
Please approve changes so I can run a test and see if there's any issues
🤔 DCMTOKEN reviewed a pull request: "contrib: Renew Windows code signing certificate"
(https://github.com/bitcoin/bitcoin/pull/30149#pullrequestreview-2256165197)
Please approve revised
(https://github.com/bitcoin/bitcoin/pull/30149#pullrequestreview-2256165197)
Please approve revised
💬 Amarrufo-1 commented on pull request "rpc: Reword SighashFromStr error message":
(https://github.com/bitcoin/bitcoin/pull/29870#issuecomment-2306293872)
I dunno wat this means
On Thu, Aug 22, 2024, 8:28 PM DCMTOKEN ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> Please approve changes so I can run a test and see if there's any issues
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2256157906>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BHYQE7MKDEWADYZXWQHIXWLZS2T53AVCNFSM6AAAAABGG2XNWCVHI2DSMVQWIX3LMV43YUDVNR
...
(https://github.com/bitcoin/bitcoin/pull/29870#issuecomment-2306293872)
I dunno wat this means
On Thu, Aug 22, 2024, 8:28 PM DCMTOKEN ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> Please approve changes so I can run a test and see if there's any issues
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/bitcoin/bitcoin/pull/29870#pullrequestreview-2256157906>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/BHYQE7MKDEWADYZXWQHIXWLZS2T53AVCNFSM6AAAAABGG2XNWCVHI2DSMVQWIX3LMV43YUDVNR
...
📝 Yihen-Liu opened a pull request: "contrib: add dockerfile for building image fron source code"
(https://github.com/bitcoin/bitcoin/pull/30702)
It may help developers save time in these aspects:
1. when developers want to deploy a test environment on a new machine, they don't need to pay too much attention to the details of the compilation environment, which is a waste of time.
2. The self-constructed image can be quickly distributed to others who want to test it without building from source code.
3. In a distributed development environment (I mean development for bitcoin core), an available dockerfile can help independent de
...
(https://github.com/bitcoin/bitcoin/pull/30702)
It may help developers save time in these aspects:
1. when developers want to deploy a test environment on a new machine, they don't need to pay too much attention to the details of the compilation environment, which is a waste of time.
2. The self-constructed image can be quickly distributed to others who want to test it without building from source code.
3. In a distributed development environment (I mean development for bitcoin core), an available dockerfile can help independent de
...
💬 maflcko commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2306347959)
> 1. when developers want to deploy a test environment on a new machine, they don't need to pay too much attention to the details of the compilation environment, which is a waste of time.
Not sure. What is the point of testing on a new machine when you fall back to a hardcoded default config? Testing is useful when variation to the underlying system and other invariants are introduced, while checking the tests still succeed.
Once you modify the dockerfile to the extent where everything is
...
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2306347959)
> 1. when developers want to deploy a test environment on a new machine, they don't need to pay too much attention to the details of the compilation environment, which is a waste of time.
Not sure. What is the point of testing on a new machine when you fall back to a hardcoded default config? Testing is useful when variation to the underlying system and other invariants are introduced, while checking the tests still succeed.
Once you modify the dockerfile to the extent where everything is
...
💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2306360216)
> don't understand why we need to refactor the whole test
Because we assume less in the test if we iterate over every block (until we still have a block reward), not just every 1000 blocks
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2306360216)
> don't understand why we need to refactor the whole test
Because we assume less in the test if we iterate over every block (until we still have a block reward), not just every 1000 blocks
💬 maflcko commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2306364968)
> I'm not quite sure what to do.
Given that the corruption happens for any kind of index (txindex, chainstate), a hardware or software issue on your side is the most likely.
I'd try to check if the internal cable is properly attached and the connector isn't dusty or dirty. Then I'd try some stuff from https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2303908456 with some caution (Backups are generally recommended, especially if data corruption is likely).
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2306364968)
> I'm not quite sure what to do.
Given that the corruption happens for any kind of index (txindex, chainstate), a hardware or software issue on your side is the most likely.
I'd try to check if the internal cable is properly attached and the connector isn't dusty or dirty. Then I'd try some stuff from https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2303908456 with some caution (Backups are generally recommended, especially if data corruption is likely).
💬 maflcko commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2306380034)
> > don't understand why we need to refactor the whole test
>
> Because we assume less in the test
I think this is a statement that is not trivial to check and probably not a good use of reviewers time. I haven't checked the history of the test case, but I presume it was intentionally written the way it looks now. For example, the `MoneyRange` check that you removed may or may not have been added to detect signed integer overflow, which is undefined behavior. I know that the `undefined` sa
...
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2306380034)
> > don't understand why we need to refactor the whole test
>
> Because we assume less in the test
I think this is a statement that is not trivial to check and probably not a good use of reviewers time. I haven't checked the history of the test case, but I presume it was intentionally written the way it looks now. For example, the `MoneyRange` check that you removed may or may not have been added to detect signed integer overflow, which is undefined behavior. I know that the `undefined` sa
...
💬 l0rinc commented on pull request "test: Improve clarity of subsidy limit test":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2306402527)
> I think this is a statement that is not trivial to check
We were checking every 1000th block, now we're checking every block.
> For example, the MoneyRange check that you removed
I have added the asserts back yesterday.
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2306402527)
> I think this is a statement that is not trivial to check
We were checking every 1000th block, now we're checking every block.
> For example, the MoneyRange check that you removed
I have added the asserts back yesterday.
💬 maflcko commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2306438172)
The link in the pull description doesn't work, and can be removed.
Also, you don't need to duplicate the full diff in the pull description.
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2306438172)
The link in the pull description doesn't work, and can be removed.
Also, you don't need to duplicate the full diff in the pull description.
💬 maflcko commented on pull request "doc: fix 3 simple CI codespell warnings":
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2306438411)
lgtm ACK cb89c8dc6a5c3fbcc73b92f4d87433c57b6ed000
(https://github.com/bitcoin/bitcoin/pull/30700#issuecomment-2306438411)
lgtm ACK cb89c8dc6a5c3fbcc73b92f4d87433c57b6ed000
💬 TheCharlatan commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1728579922)
I've been looking the logic over again and think it might be a good idea to call `NotifyHeaderTip` here too. It basically tracks along `m_best_header`, but with the current code will only notify when processing the next block or header.
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1728579922)
I've been looking the logic over again and think it might be a good idea to call `NotifyHeaderTip` here too. It basically tracks along `m_best_header`, but with the current code will only notify when processing the next block or header.
💬 sipsorcery commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2306583830)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4.
Tested successful builds on:
- Win11 + msvc x64 dynamic and static
- Win11 + WSL Ubuntu 24.04 + mingw32
I couldn't perform the build on Win11 + WSL + Ubuntu 20.04 as the g++-mingw toolset not available. I don't think this is a problem since it's trivial to install a newer Ubuntu version on WSL.
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2306583830)
ACK 41051290ab3b6c36312cec26a27f787cea9961b4.
Tested successful builds on:
- Win11 + msvc x64 dynamic and static
- Win11 + WSL Ubuntu 24.04 + mingw32
I couldn't perform the build on Win11 + WSL + Ubuntu 20.04 as the g++-mingw toolset not available. I don't think this is a problem since it's trivial to install a newer Ubuntu version on WSL.
💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306601521)
The cases where enabling full-rbf causes problems are very rare: you need to be running a service that is _not_ vulnerable to unconfirmed double-spends in general. Yet _does_ have some kind of bug related to double spends. And those rare services can still easily turn full-rbf off with a single config setting.
Meanwhile, the harm to compact block propagation caused by not enabling full-rbf can be seen as a bug that should be fixed.
(https://github.com/bitcoin/bitcoin/pull/30558#issuecomment-2306601521)
The cases where enabling full-rbf causes problems are very rare: you need to be running a service that is _not_ vulnerable to unconfirmed double-spends in general. Yet _does_ have some kind of bug related to double spends. And those rare services can still easily turn full-rbf off with a single config setting.
Meanwhile, the harm to compact block propagation caused by not enabling full-rbf can be seen as a bug that should be fixed.
💬 glozow commented on pull request "contrib: add dockerfile for building image fron source code":
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2306619681)
> * In a distributed development environment (I mean development for bitcoin core), an available dockerfile can help independent developers align the compilation environment.
I'm pretty sure dev environment convergence is something we explicitly *don't* want for Bitcoin Core.
> In any case, have you seen the existing CI dockerfiles, which are probably what you are looking for? If not, it would be good to explain the difference.
+1
(https://github.com/bitcoin/bitcoin/pull/30702#issuecomment-2306619681)
> * In a distributed development environment (I mean development for bitcoin core), an available dockerfile can help independent developers align the compilation environment.
I'm pretty sure dev environment convergence is something we explicitly *don't* want for Bitcoin Core.
> In any case, have you seen the existing CI dockerfiles, which are probably what you are looking for? If not, it would be good to explain the difference.
+1
💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2306678413)
`1dcd626fe1...587d996f07`: rebase, get the extra printout from https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2245216013 and resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/28052
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2306678413)
`1dcd626fe1...587d996f07`: rebase, get the extra printout from https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2245216013 and resolve a silent merge conflict with https://github.com/bitcoin/bitcoin/pull/28052
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2306692234)
> TESTED_TX_RESULTS is missing TxValidationResult::TX_RECONSIDERABLE
That's an oversight, I've added that and UNKNOWN now which should mean we see packages.
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2306692234)
> TESTED_TX_RESULTS is missing TxValidationResult::TX_RECONSIDERABLE
That's an oversight, I've added that and UNKNOWN now which should mean we see packages.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728679985)
I wrote the assertion incorrectly. The only combination that's not ok is both.
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728679985)
I wrote the assertion incorrectly. The only combination that's not ok is both.
💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728680149)
added
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1728680149)
added