🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2560158721)
Thanks for the reviews and testing!
Rebased 044c1129db06983da598f427dff85513d8480b3a -> 17f25149c680c1fdb2f79dbe373e8cf820e62488 ([`pr/wrap.11`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.11) -> [`pr/wrap.12`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.11-rebase..pr/wrap.12)) to fix conflicts, also made many suggested updates and some fixes, particularly for windows.
---
re: https://github.com/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2560158721)
Thanks for the reviews and testing!
Rebased 044c1129db06983da598f427dff85513d8480b3a -> 17f25149c680c1fdb2f79dbe373e8cf820e62488 ([`pr/wrap.11`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.11) -> [`pr/wrap.12`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.11-rebase..pr/wrap.12)) to fix conflicts, also made many suggested updates and some fixes, particularly for windows.
---
re: https://github.com/bitcoi
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920878009)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893661643
Removed from documentation for now since maybe this isn't something we want to support going forward, and in any case I think it would be better to document internal commands for debugging and testing and external commands intended to be used by typical users separately.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920878009)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893661643
Removed from documentation for now since maybe this isn't something we want to support going forward, and in any case I think it would be better to document internal commands for debugging and testing and external commands intended to be used by typical users separately.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920881130)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893680784
Thanks, fixed now
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920881130)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893680784
Thanks, fixed now
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920883094)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893702291
Thanks, added doxygen documentation and descriptions.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920883094)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893702291
Thanks, added doxygen documentation and descriptions.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920883201)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893709992
> I find the names confusing because I would assume this will execute `argv0 args[0] args[1] args[2] ...`. Maybe rename `argv0` to `parent` or `wrapper_prog`.
Makes sense, and nice suggestion. renamed to wrapper_argv0. Also renamed other related variables to use wrapper_ prefix to be consistent.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920883201)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893709992
> I find the names confusing because I would assume this will execute `argv0 args[0] args[1] args[2] ...`. Maybe rename `argv0` to `parent` or `wrapper_prog`.
Makes sense, and nice suggestion. renamed to wrapper_argv0. Also renamed other related variables to use wrapper_ prefix to be consistent.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920879100)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893670004
> So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either bitcoin -M gui or bitcoin -m gui. Could we spare them this? The bitcoin executable has that knowledge.
No this isn't how it works. Specifying -m or -M is optional, and the current default is to run monolithic binaries. Multiprocess binaries could become the default at some some point in the future b
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920879100)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893670004
> So, the user must have a knowledge whether his Bitcoin Core was compiled with or without multiprocess and start either bitcoin -M gui or bitcoin -m gui. Could we spare them this? The bitcoin executable has that knowledge.
No this isn't how it works. Specifying -m or -M is optional, and the current default is to run monolithic binaries. Multiprocess binaries could become the default at some some point in the future b
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920882677)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893696352
> This adds a trailing newline, but the above 2 `tfm::format()` calls don't. I think they should.
The output looks right to me currently and trailing newlines are not missing. Adding \n to format strings would add double line breaks to the end of output and not be right, and in general there should be no need to include \n in format strings after substituting variables that already end in newlines.
If the idea is t
...
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920882677)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893696352
> This adds a trailing newline, but the above 2 `tfm::format()` calls don't. I think they should.
The output looks right to me currently and trailing newlines are not missing. Adding \n to format strings would add double line breaks to the end of output and not be right, and in general there should be no need to include \n in format strings after substituting variables that already end in newlines.
If the idea is t
...
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920891035)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893800793
> Isn't `fs::exists()` redundant? `fs::is_regular_file()` will return true if the file exists and is a regular file.
Good catch, simplified.
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920891035)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893800793
> Isn't `fs::exists()` redundant? `fs::is_regular_file()` will return true if the file exists and is a regular file.
Good catch, simplified.
💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920891135)
> re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893808229
Thanks fixed
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1920891135)
> re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1893808229
Thanks fixed
💬 theStack commented on pull request "rpc: allow writing UTXO set to a named pipe, introduce dump_to_sqlite.sh script":
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2599524193)
> Can we make the exists/is_fifo/open atomic somehow? Seems liable to have a race here someday...
> eg [luke-jr@56ee485](https://github.com/luke-jr/bitcoin/commit/56ee485533820d8c7f0e4e27acd712aa8b411286)
@luke-jr: Thanks, applied this to https://github.com/bitcoin/bitcoin/pull/31560/commits/292b0e884df3f266fc57d583cca46ca80400fa4a. Note that I had to introduce a `fs::exists` helper for file_status, as direct usage of `std::filesystem::exists` was prohibited by the linter (which failed wit
...
(https://github.com/bitcoin/bitcoin/pull/31560#issuecomment-2599524193)
> Can we make the exists/is_fifo/open atomic somehow? Seems liable to have a race here someday...
> eg [luke-jr@56ee485](https://github.com/luke-jr/bitcoin/commit/56ee485533820d8c7f0e4e27acd712aa8b411286)
@luke-jr: Thanks, applied this to https://github.com/bitcoin/bitcoin/pull/31560/commits/292b0e884df3f266fc57d583cca46ca80400fa4a. Note that I had to introduce a `fs::exists` helper for file_status, as direct usage of `std::filesystem::exists` was prohibited by the linter (which failed wit
...
📝 Ali-pixcell opened a pull request: "Delete .github/workflows directory"
(https://github.com/bitcoin/bitcoin/pull/31680)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31680)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
✅ Ali-pixcell closed a pull request: "Delete .github/workflows directory"
(https://github.com/bitcoin/bitcoin/pull/31680)
(https://github.com/bitcoin/bitcoin/pull/31680)
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2599599923)
Rebased 538671edce5813a62405b9bd5c50c39263c58435 -> 01a43b24436e0aed7b8f79d3857630a4bf6a0545 ([kernelApi_17](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_17) -> [kernelApi_18](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_18), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_17..kernelApi_18))
* Get functional test fix from #31675
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2599599923)
Rebased 538671edce5813a62405b9bd5c50c39263c58435 -> 01a43b24436e0aed7b8f79d3857630a4bf6a0545 ([kernelApi_17](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_17) -> [kernelApi_18](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_18), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_17..kernelApi_18))
* Get functional test fix from #31675
📝 fanquake locked a pull request: "Delete .github/workflows directory"
(https://github.com/bitcoin/bitcoin/pull/31680)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/31680)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
⚠️ SatoshiNakamoto2007 opened an issue: "BitcoinBinanceBank "
(https://github.com/bitcoin/bitcoin/issues/31681)
(https://github.com/bitcoin/bitcoin/issues/31681)
✅ fanquake closed an issue: "BitcoinBinanceBank "
(https://github.com/bitcoin/bitcoin/issues/31681)
(https://github.com/bitcoin/bitcoin/issues/31681)
:lock: fanquake locked an issue: "BitcoinBinanceBank "
(https://github.com/bitcoin/bitcoin/issues/31681)
(https://github.com/bitcoin/bitcoin/issues/31681)
💬 Eunovo commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2599687406)
I had an offline conversation with @mzumsande. During the reindex, we can apply assumevalid for following cases:
- If the assumevalid block is the index, then we can skip script checks for all ancestors of the assumevalid block
- If the assumevalid block is not in the index, then the initial IBD run had not downloaded the assumevalid block yet then the blocks on disk are ancestors of the assumevalid block and we can still skip script verification
Then we download headers from peers in paral
...
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-2599687406)
I had an offline conversation with @mzumsande. During the reindex, we can apply assumevalid for following cases:
- If the assumevalid block is the index, then we can skip script checks for all ancestors of the assumevalid block
- If the assumevalid block is not in the index, then the initial IBD run had not downloaded the assumevalid block yet then the blocks on disk are ancestors of the assumevalid block and we can still skip script verification
Then we download headers from peers in paral
...
📝 Eunovo converted_to_draft a pull request: "Ensure assumevalid is always used during reindex"
(https://github.com/bitcoin/bitcoin/pull/31615)
assumevalid is not always used during reindex because the chainwork of the best header might be less than the minimumchainwork if the previous IBD was interrupted before it could connect blocks up to minimumchainwork.
See Issue https://github.com/bitcoin/bitcoin/issues/31494
This does not occur during normal IBD because the node does not connect blocks before minchainwork headers.
(https://github.com/bitcoin/bitcoin/pull/31615)
assumevalid is not always used during reindex because the chainwork of the best header might be less than the minimumchainwork if the previous IBD was interrupted before it could connect blocks up to minimumchainwork.
See Issue https://github.com/bitcoin/bitcoin/issues/31494
This does not occur during normal IBD because the node does not connect blocks before minchainwork headers.
💬 glozow commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#issuecomment-2599722690)
Rebased for #31675 which fixes https://cirrus-ci.com/task/5200590696873984
(https://github.com/bitcoin/bitcoin/pull/31666#issuecomment-2599722690)
Rebased for #31675 which fixes https://cirrus-ci.com/task/5200590696873984