[kernel 0/n] Introduce bitcoin-chainstate
#24304
Conversation
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
2 times, most recently
from
c7bcdbe
to
518c876
Compare
| [](){ return false; }); | ||
| if (rv.has_value()) { | ||
| std::cerr << "Failed to load Chain state from your datadir." << std::endl; | ||
| goto epilogue; |
There was a problem hiding this comment.
Wouldn't the code be easier to read by replacing this with return; and letting a destructor of an object that lives in this scope take care of the cleanup?
There was a problem hiding this comment.
Unfortunately, with our codebase right now if we don't do the shutdown process in a very carefully sequenced way, there will be nullptr dereferences and other undefined behaviour.
There was a problem hiding this comment.
I don't mean to reorder the sequence. It is just a suggestion to remove goto, which in my eyes is impossible to review. I am convinced that any code using goto can be rewritten in a way that doesn't use goto and is at the same time easier to read. I can look into this in the future, unless someone beats me to it.
There was a problem hiding this comment.
I don't mean to reorder the sequence. It is just a suggestion to remove
goto, which in my eyes is impossible to review.
IMO, it is ok to use goto in a limited way to exit a block, and not worse than break. This can be cleaner and better than more convoluted alternatives and there are good practices build around it https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
There was a problem hiding this comment.
I don't think we should be using the style guide of the kernel (a project written in C) for a project written in C++.
But given that the code here isn't used for anything after compilation (other than to prove that linking works), I think any style is fine here.
There was a problem hiding this comment.
IMO goto is acceptable in C, which has no RAII and other scope-based ways of handling errors, but not in C++, generally. The only current use of goto we have in the code is in src/crypto/poly1305.cpp which is effectively included C code. So i'd really like to avoid it if remotely possible.
There was a problem hiding this comment.
In commit "build: Add example bitcoin-chainstate executable" (ac43dd0)
I guess the goto is kind of a shiny object. You can get rid of gotos by improving code (refactoring out functions, using RAII) but you can also get rid of gotos by making code worse (adding layers of nesting and loops and confusing variables). Exciting to see what is in this little goto's future!
There was a problem hiding this comment.
Agree that this goto is a shiny object 😅 . It's actually the reason why I added the developer note at the beginning of the file here:
bitcoin/src/bitcoin-chainstate.cpp
Lines 9 to 10 in 095aa6c
I think that since this binary is a "demo-only" executable (that will actually later be moved to src/kernel/examples/bitcoin-chainstate) we can not bother too much with its style. However, I do agree that this wouldn't be acceptable if it were not "demo-only".
If people feel very strongly about it, please speak up and I can make the necessary modifications, but it might make things a bit less readable.
There was a problem hiding this comment.
Here's a simple way of avoiding goto without (I think) making things less readable: ajtowns@5d7d8de
There was a problem hiding this comment.
@ajtowns Hmmm good point... Well since I'm going to introduce a kernel::Context object anyway, might be good to just move the init/shutdown logic over there and let RAII handle it. Will play around with it, thanks for the hint!!
Do you need to add one? What about adding it to the existing nowallet task? |
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
from
518c876
to
1d9faa8
Compare
|
Pushed 518c876...1d9faa8: |
I'm happy to add it to the existing nowallet task if you think it's appropriate! Lmk! |
Yeah, I don't think there are any downsides and only upsides to it, right? |
|
From ci: |
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
5 times, most recently
from
7c40e96
to
d8f2e52
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Code review ACK e5579f1 with caveat that I mostly focused on the first commit and did not very closely review the build changes. (I think it could be a good idea to put first commit and other commits into two separate PRs, because the reviewers who are best able to give feedback about the code in the first commit are not necessarily the same people who can provide best feedback about the build commits.)
src/bitcoin-chainstate.cpp
Outdated
|
|
||
| ChainstateManager chainman; | ||
|
|
||
| auto rv = LoadChainstate(false, |
There was a problem hiding this comment.
In commit "build: Add example bitcoin-chainstate executable" (01840e6)
The LoadChainstate and VerifyLoadedChainstate calls here would be cleaner with b7c7f64 suggested in #23280 (review) and would let this code print specific error messages instead failing generically and discarding the return codes.
| [](){ return false; }); | ||
| if (rv.has_value()) { | ||
| std::cerr << "Failed to load Chain state from your datadir." << std::endl; | ||
| goto epilogue; |
There was a problem hiding this comment.
I don't mean to reorder the sequence. It is just a suggestion to remove
goto, which in my eyes is impossible to review.
IMO, it is ok to use goto in a limited way to exit a block, and not worse than break. This can be cleaner and better than more convoluted alternatives and there are good practices build around it https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
3 times, most recently
from
91bf084
to
c5d2125
Compare
dongcarl
changed the title
bitcoin-chainstate and libbitcoinkernelbitcoin-chainstate
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
2 times, most recently
from
da5937d
to
5fb9086
Compare
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
2 times, most recently
from
93f0dec
to
0ad6a3e
Compare
|
Pushed 0ad6a3e:
|
|
Assigned 24.0, as this missed the 0.23 feature freeze |
There was a problem hiding this comment.
re-ACK 0ad6a3e 🔟
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 0ad6a3e81265ef2ad5353c4e0f6553955980d2bc 🔟
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjLOgwAjdvJIwihxHSRaKHl0iVEsfH9gOsvngLNiBtqBBlXbooA0NFaFXMKce5f
v17lIZqfNLCfyIEZFSrgb4cTRAMURDJNTyVg4pFBCEVcmSxmcGniff8v9Y+pbPj7
Z7WYljWzVJpZeuAQyFwiAr9AerIYUZVndLX5X+QKf+JMBqg1Vv7G7SIfgdlUAzl8
+hIPW144xzl6xZRsTJrBp91GdAAY/vyAz+8EfFay82SAJC6ZE9CGtUEzrYyOQ8S/
vD+5ach5xi2FbOQgXYX+49Hqgx+x0VzAhEI4XeUm0NDKaiRlK15oPegyPXjL+obT
wmE+N1AzpeYJ60Wih64XyT7NB3DaMpExoybmMlgPaOyfLZvC+nMlhjffPG5M3sni
Xl1ffLN873bQgYJf8mT/vO/U+PHTD6iJBk9LzhMYTF5ltW94j7/2XCCB3/3nRCFn
2R0jz8wJxovw2a/MDkI3mdfvzzWQeIugGNZie2fs+Vtn5cgyWfdburKq4d2bUEum
MuiehY0A
=PeLI
-----END PGP SIGNATURE-----
| // now. | ||
| // | ||
| // DEVELOPER NOTE: Since this is a "demo-only", experimental, etc. executable, | ||
| // it may diverge from Bitcoin Core's coding style. |
There was a problem hiding this comment.
I think this can be removed and if there was any reason to use a different coding style for this binary, it can be explained in the pull request, a review comment, or a code comment.
There was a problem hiding this comment.
Some discussion here: #24304 (comment)
There was a problem hiding this comment.
Code review ACK 0ad6a3e with same caveat that mostly focused on first commit, paid less attention to build changes in second commit. Only changes since last review were documentation changes which all looked very good to me.
Some comments about dependencies were lost, but it is probably better not to have the level of detail they were going into here:
- // Initialize signatureCache cuz it's used by...
- // <- VerifyECDSASignature
- // <- CheckECDSASignature
- // <- EvalChecksigPreTapscript
- // <- EvalScript
- // <- VerifyScript
- // <- CScriptCheck::()
- // <- CheckInputScripts
- // <- ConnectBlock
- // <- ConnectTip
- // <- ActivateBestChainStep
- // <- ActivateBestChain
- // <- ProcessNewBlock
InitSignatureCache();
-
- // Initialize g_scriptExecutionCache{,Hasher} cuz it's used by...
- // <- CheckInputScripts
- // <- ConnectBlock
- // <- ConnectTip
- // <- ActivateBestChainStep
- // <- ActivateBestChain
- // <- ProcessNewBlock
InitScriptExecutionCache();| [](){ return false; }); | ||
| if (rv.has_value()) { | ||
| std::cerr << "Failed to load Chain state from your datadir." << std::endl; | ||
| goto epilogue; |
There was a problem hiding this comment.
In commit "build: Add example bitcoin-chainstate executable" (ac43dd0)
I guess the goto is kind of a shiny object. You can get rid of gotos by improving code (refactoring out functions, using RAII) but you can also get rid of gotos by making code worse (adding layers of nesting and loops and confusing variables). Exciting to see what is in this little goto's future!
The bitcoin-chainstate executable serves to surface the dependencies
required by a program wishing to use Bitcoin Core's consensus engine as
it is right now.
More broadly, the _SOURCES list serves as a guiding "North Star" for the
libbitcoinkernel project: as we decouple more and more modules of the
codebase from our consensus engine, this _SOURCES list will grow shorter
and shorter. One day, only what is critical to our consensus engine will
remain. Right now, it's "the minimal list of files to link in to even
use our consensus engine".
[META] In a future commit the libbitcoinkernel library will be extracted
from bitcoin-chainstate, and the libbitcoinkernel library's
_SOURCES list will be the list that we aim to shrink.
...to make sure that the linker errors that arise from coupling regressions are caught by CI. Adding to the "no wallet" ci job as suggested by MarcoFalke.
dongcarl
force-pushed
the
2022-02-libbitcoinkernel-p0-minimal
branch
from
0ad6a3e
to
2c03cec
Compare
|
Pushed 2c03cec
|
There was a problem hiding this comment.
Did some light testing of 2c03cec. Very cool! I fed it block 1 and 2 on a fresh datadir and it was happy. I fed it a duplicate block and it exited. I fed it block 4 and it complained it didn't have the previous block. All fine for now I would say.
Can you add a short documentation, e.g. contrib/KERNEL.md, that includes a link to #24303, the configure flag and one example incantation how to use this binary? It doesn't have to explain the entire kernel project.
It might also be good to add a basic functional test for the binary. test/functional/tool_wallet.py should be a good template. You may need to add something to good_prefixes_re depending on how you name the test file.
A skeleton manpage might be nice too, see plumbing example in #20913.
| @@ -626,6 +627,12 @@ AC_ARG_ENABLE([util-util], | |||
| [build_bitcoin_util=$enableval], | |||
| [build_bitcoin_util=$build_bitcoin_utils]) | |||
|
|
|||
| AC_ARG_ENABLE([experimental-util-chainstate], | |||
| [AS_HELP_STRING([--enable-experimental-util-chainstate], | |||
There was a problem hiding this comment.
You may want to document this in the PR description and elsewhere.
Also should be in the ./configure summary.
There was a problem hiding this comment.
re: #24304 (comment)
You may want to document this in the PR description and elsewhere.
Also should be in the
./configuresummary.
I believe this is only temporarily built as part of our build system and later moves to an examples folder (and built with cmake!) So that may a reason not to add it to --configure help, though I guess it you could have a comment saying the option is temporary and will be removed later. Agree it makes sense to document in PR description and elsewhere (maybe at the top of bitcoin-chainstate.cpp) so it's obvious how to build & test this
There was a problem hiding this comment.
So that may a reason not to add it to --configure help,
This option is already included in ./configure --help output, that's done automatically by autotools.
Also should be in the ./configure summary.
I don't think it needs to be.
There was a problem hiding this comment.
Can you add a short documentation, e.g.
contrib/KERNEL.md, that includes a link to #24303, the configure flag and one example incantation how to use this binary? It doesn't have to explain the entire kernel project.
You already know this since you commented there, but for anyone else I started writing up some things about libbitcoinkernel design in #24352 (with a lot of "probably" hedging which I'll remove because it was before I talked to Carl and was guessing about some things).
I think existing https://github.com/bitcoin/bitcoin/blob/master/doc/shared-libraries.md will be a good place to document libbitcoinkernel usage when it gets added by the next PR.
It might also be good to add a basic functional test for the binary.
test/functional/tool_wallet.pyshould be a good template. You may need to add something togood_prefixes_redepending on how you name the test file.A skeleton manpage might be nice too, see plumbing example in #20913.
I think this is only temporarily built as part of our build system, and later moving to an examples folder with a CMake file, so that would be a reason not to do the test, manpage, etc in this style.
| @@ -626,6 +627,12 @@ AC_ARG_ENABLE([util-util], | |||
| [build_bitcoin_util=$enableval], | |||
| [build_bitcoin_util=$build_bitcoin_utils]) | |||
|
|
|||
| AC_ARG_ENABLE([experimental-util-chainstate], | |||
| [AS_HELP_STRING([--enable-experimental-util-chainstate], | |||
There was a problem hiding this comment.
re: #24304 (comment)
You may want to document this in the PR description and elsewhere.
Also should be in the
./configuresummary.
I believe this is only temporarily built as part of our build system and later moves to an examples folder (and built with cmake!) So that may a reason not to add it to --configure help, though I guess it you could have a comment saying the option is temporary and will be removed later. Agree it makes sense to document in PR description and elsewhere (maybe at the top of bitcoin-chainstate.cpp) so it's obvious how to build & test this
I agree with Russ. I don't really see a reason to add tests, let alone a manpage for this binary. |
Having untested examples could lead to code rot though. I could also imagine this (ZMQ of course doesn't belong in a kernel, but some sort of notification interface probably does, and this binary can demo that) |
Agreed. I definitely think this should be continue to be built by CI at the very least, and maybe a have a little script feeding it a test block.
This could be great, but I believe it should be a separate tool cloned / inspired from this one. When you are using a new library it's helpful to have a simple minimal example along with the fancy demo example. |
Stdout is my favorite notification interface 😏 |
|
Not sure if this needs a manpage and test, since there is already a test for it (see second commit). The goal of the binary right now is to confirm that the compiler and linker doesn't fail. I don't think that the binary should be considered stable for end users. Also, the interface is mostly a playground, as I understand it, and not the interface that will be shipped when it is final and ready for end users. Thus, adding a test will make it harder to change the interface later on. |
There was a problem hiding this comment.
re-ACK 2c03cec 🏔
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
re-ACK 2c03cec2ff8cdbfd5da92bfb507d218e5c6435b0 🏔
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjqVQv9GTYBZ644zKBR/8+NgZAZqR/vzNuVLzgPo9UWDexF6M1LZRVpQaoaz831
PxJui5vAF4Eegt6OPhOEzmW4jpd6s5MpURCBL8BzKiyRxe0uT0MPnzufwpZn3iBP
SVpmoAcgG0ZjM9cdY2iJsLqhf47AHqAqh2K8f9gCoObqloHgjIb5fiK5gXowWLDd
B/zlZgmIaxlS/rgyUkpLa/5kIEOONyO56abJ9PhAlXYKm1GIX5+/GgB0Yb/BJEwL
bP843qBy511C07usDLKp3pPMPAFakDFJPJlmE7evSE5DEKcoXVTQGTjoK1JQ/j4t
Pa2bESI48/V7dBOGYDsEd0AdoW3J2Mkhkbzk1lp6Fu+vCQQbEqIVw5F05p3hUmuJ
RyDJROFVjvONophU2MMLOkL7xlfmNQzXY56ONDsd3OhPWG7bX82Kf0XPZ5rDqDGn
9FWDiAf5/mTscazQ0wcwLZlIHX9gx0v0XOZNcLiStfaeAPq4X3U2cEN+2oPHHQtg
8CMkgV6s
=nVUh
-----END PGP SIGNATURE-----
|
ACK 2c03cec |
|
Post-merge ACK 2c03cec |
035fa1f build: Remove LIBTOOL_APP_LDFLAGS for bitcoin-chainstate (Cory Fields) 3f05950 docs: Add libbitcoinkernel_la_SOURCES explanation (Carl Dong) 94ad45d ci: Build libbitcoinkernel (Carl Dong) 26b2e7f build: Extract the libbitcoinkernel library (Carl Dong) 1df44dd b-cs: Define G_TRANSLATION_FUN in bitcoinkernel.cpp (Carl Dong) 83a0bb7 build: Separate lib_LTLIBRARIES initialization (Carl Dong) c1e16cb build: Create .la library for bitcoincrypto (Carl Dong) 8bdfe05 build: Create .la library for leveldb (Carl Dong) 05d1525 build: Create .la library for crc32c (Carl Dong) 64caf94 build: Remove vestigial LIBLEVELDB_SSE42 (Carl Dong) 1392e8e build: Don't add unrelated libs to LIBTEST_* (Carl Dong) Pull request description: Part of: #24303 This PR introduces a `libbitcoinkernel` static library linking in the minimal list of files necessary to use our consensus engine as-is. `bitcoin-chainstate` introduced in #24304 now will link against `libbitcoinkernel`. Most of the changes are related to the build system. Please read the commit messages for more details. ACKs for top commit: theuni: This may be my favorite PR ever. It's a privilege to ACK 035fa1f. Tree-SHA512: b755edc3471c7c1098847e9b16ab182a6abb7582563d9da516de376a770ac7543c6fdb24238ddd4d3d2d458f905a0c0614b8667aab182aa7e6b80c1cca7090bc
Part of: #24303
This PR introduces an example/demo
bitcoin-chainstateexecutable using said library which can print out information about a datadir and take in new blocks on stdin.Please read the commit messages for more details.
You may ask: WTF?! Why is
index/*.cpp, etc. being linked in?This PR is meant only to capture the state of dependencies in our consensus engine as of right now. There are many things to decouple from consensus, which will be done in subsequent PRs. Listing the files out right now in
bitcoin_chainstate_SOURCESis purely to give us a clear picture of the task at hand, it is not to say that these dependencies belongs there in any way.TODO
bitcoin-chainstate.cppIt is quite ugly, with a lot of comments I've left for myself, I should clean it up to the best of my abilities (the ugliness of our init/shutdown might be the upper bound on cleanliness here...)