chaincodelabs / onboarding-to-bitcoin-core Goto Github PK

Not sure if this is just a difference in OS.

I noticed ../.autogen.sh, on the latest pull, is just named ../autogen.sh for me on MacOs.
Same with the configure commands ../configure --regular options

Problem:

This repo currently does not have any CI/CD. It is a manual build and deploy process. If the person(s) responsible for managing this repo are unavailable then no code changes can be deployed.

Suggestion:

1. Enable GitHub pages for the main/master branch for this repo
2. Point the CNAME subdomain records to the Github location. Read the docs here
3. Setup Github Actions to run the build command asciidoctor -r asciidoctor-diagram index.adoc. Something like this could be a good starting point

Alternate Suggestion:

If steps 1 and 2 are inappropriate then Github Actions could still be used to deploy the built index.html to the current hosting location, but will require some extra Github Actions and some access keys to be added to this repository secrets under the admin Settings page.

We could also add a section on test coverage.

This is tracked here and can be a good way for new contributors to "find something useful" to improve?

In section-1 under the heading Organisation fail-safe:
Though the link says “Discussion about moving away from bitcoin,” I think the link provided, though it is a discussion, has many parts irrelevant to the link’s topic. I would suggest two alternatives:

• Replacing the link with https://github.com/bitcoin-core/bitcoin-devwiki/wiki/GitHub-alternatives-for-Bitcoin-Core to give the summarised version of the discussion till now.
• Adding the above link along with the original discussion link to give the reader the gist of both the discussion process and a summarised read of the discussion’s result.

Overall, great doc. I'll capture general feedback here and also open a PR for nits/spelling.

Overview

• BerkeleyDB is unmaintained and on a deprecation path from Core. Might be worth linking to the proposed deprecation plan: bitcoin/bitcoin#20160

• IIRC, sqlite wallets cannot create legacy wallets, only descriptors (it's enforced when creating wallets BDB is on a deprecation path - might be worth mentioning and also
  adding a link to the deprecation timeline

Wallet Architecture

• in wallet/ doc, perhaps mention bech32m? i believe it will be a supported descriptor in the next release (23.0). Ping @lsilva01

Coin Selection (AvailableCoins)

• There are three coin selection algos, no? BnB, Knapsack, SRD
• When explaining coin selection, I think it's more accurate to say that BnB, Knapsack, and SRD are all run, and then the most optimal one is chosen based on the WasteMetric - in simple terms, wasteMetric calculates an opportunity cost of spending now vs how expensive this would be to spend later at 10 s/vb (paraphrashing)
• Might also be worth explaining the waste metric in this section
• It is inaccurate to say Knapsack is a fallback, I think knapsack gets chosen 60% of the time over BnB (sauces)
• Might be worth explaining output groups, as this is a pretty big section of the coinselection.cpp / spend.cpp codebase and it can be very confusing reading through the code if you don't know what an outputGroup is or what they are solving for

CWallet

• re: birthday, you can also force a wallet to rescan the whole chain
• how do birthdays work with imported wallets?
• not sure which is the most "correct" terminology, but id avoid referencing "addresses". perhaps output types, or script types?
• even better, write a section about how these concepts map to each other w.r.t the wallet (address <-> output type <-> scripts)

Constructing Transactions

• I have some notes on how AvailableCoins works, ill send them. Yours are mostly correct mod a few nits
• interestingly, CWallet::mapWallet operates on TXs, not Coins. it first checks of the TX is eligible, then loops through each Output of the tx and adds them to available coins
• AvailableCoins also takes a coincontrol object and uses it while deciding which coins are available, might be worth mentioning here

HWI

hmu when you get to this section, happy to help/review! I use it in my setup and have also dug into the internals for the 22.0 testing guide

The guide mentions that

they should be determining the mergability of changes based primarily on the reviews and discussions of other Contributors on the GitHub PR, or less commonly, the #bitcoin-core-dev mailing list.

The bitcoin-core-dev list has been a notification-only list for a while. Only moderators can post. This is also reflected in the blurb. it was never used for discussion while it could be, so at some point we decided to change the purpose.

There's some discussion on the bitcoin-dev (no -core) list but this usually applies to topics that generally affect bitcoin, not bitcoin-core specific project issues. There's only github and the #bitcoin-core-dev IRC channel for that. Do you maybe mean that?

When using:

# Clean current source dir in case it was already configured
make distclean

# Make new build dir
mkdir build && cd build

# Run normal build sequence with amended path
../autogen.sh
../configure --your-normal-options-here
make -j `nproc

test_runner.py is copied to the bitcoin/build/test/funcional directory, but not all the individual functional tests, the file config.ini gets generated in bitcioin/build/test directory, allowing test_runner.py to be run with test/fuctional/test_runner.py from the bitcoin/build directory without any problem.

However, if you want to run the individual functional test, you have to do it from the bitcoin/test/functional directory, but if you do that, it cannot find the config.ini file. Even if you run ../../test/functional/p2p_headers_sync_with_minchainwork.py from the bitcoin/build/test directory, where config.ini is located, it's looking for it on the bitcoin/test subdirectory.

FileNotFoundError: [Errno 2] No such file or directory: '/Users/... .../bitcoin/test/config.ini'

via sipa:

"first party malleation: signature length descriptor is extended to 5 bytes".

"First party malleability" is kind of weird term... the signer doesn't really need to malleate - they can also just create another signature."

You could move sections like:

• Decentralized Dev
• Bitcoin Core Dev Process and Doc
• Reviews
• Commit Messages
• Continuous Integration
• Getting Started with Development

Into a new 02_development-process.adoc, and the remaining material in the current 02 renamed to 03, update number accordingly. Exercises in 01 that are related to the development process can be moved to this new section.

After doing this, the overview-and-architecture section can now be focused on just that, and material within it expanded on.

We should find a way to work #13 (comment) in.

@LarryRuane

This document as well as https://github.com/chaincodelabs/bitcoin-core-onboarding are based on a specific reference of the codebase. Throughout the onboarding documentation there are links that are referenced more than once. I was thinking that moving all the links to variables would remove duplicate links but more importantly would make easier any additions to the documentation along with easier upgradability in case of future migration to a newer version of the codebase.

I've started playing along with the idea, initially for Regions but as it is referencing the same commit, is easy to just have the same source of truth for everything. My idea is something like

:commit: 4b5659c6b115315c9fd2902b4edd4b960a5e066e
:link: https://github.com/bitcoin/bitcoin/blob/{commit}/src

// files
:net_processing_h: {link}/net_processing.h
:net_processing_cpp: {link}/net_processing.cpp
:validation_cpp: {link}/validation.cpp
:validation_h: {link}/validation.h
...
// net_processing.{h,cpp}
:PeerManagerImpl: {net_processing_cpp}#L227
:m_chainman: {net_processing_cpp}#L335
...
// validation.{h,cpp}
:ConnectBlock: {validation_cpp}#L1802
:ConnectTip: {validation_cpp}#L2460
:ActivateBestChainStep: {validation_cpp}#L2596

which could be imported on every doc with

include::links.adoc[]

The only problem with this is that including asciidoc files is not working on GitHub which is an issue for people reading the documentation from within GitHub and not from the website. To be able to work for GitHub it would need all the variables to be presented in each file, which complicates the maintenance process. A solution to that could be adding asciidoctor-reducer as a pre-commit hook.

I wanted to get some feedback to understand if others see value on this and if the GitHub limitation is consider a forbidding factor.

Currently the whole website is "checkpointed" at one commit ("This document was created from Bitcoin Core at commit 4b5659c6").

This may be good enough for now, but as the website gets increasingly outdated, it may discourage people from updating it, since it requires the entirety to be updated. Maybe it would be better to "checkpoint" each chapter separately, so that (for example) wallet people can update "their" chapter while leaving other chapters untouched?

This is a feature request split into two parts:

1. Add a glossary of the abbreviations used throughout the document
2. As a follow up, add tooltips to enable mouse over interactions that quickly display the full expansion of the abbreviations (or something similar)

In the current docs there is a sentence:

There is a PR [open](https://github.com/bitcoin/bitcoin/pull/19602) which aims to permit migration of legacy wallets → descriptor form. But for now (2022) it’s still relevant to have stuff related to legacy wallet in documentation.

This PR is now merged: bitcoin/bitcoin#19602

It's probably worth adding a different explanation there about the migration of legacy wallets?

In Tests overview, it would be great to short mention we have stress test in Core? It's into the functional test (to not rewrite same code) but we can consider it a different category of test.

See: bitcoin/bitcoin@d9803f7

The build system is an integral part of bitcoin core. Priorities for the build system and what dependencies we want bitcoin core to use have historically led to heavy refactoring of the code. The philosophy behind the build system also touches on ideas of development behind bitcoin core is to be treated as is security oriented software. It is also how we get reproducible binaries which is pretty important to talk about.

Looking for concept ack as it could be deemed info on the build system is not necessary for onboarding. Happy to take this on and open a PR if consensus.

https://x.com/plightus/status/1770046686997479764

• I have prepared a list of questions related to the current architecture of Bitcoin core.
• I used this documentation for reference in designing questions.
• The questions are arranged in chronological order based on where they come in the documentation.
• For now, I am putting the list of questions as an issue so that we can select the questions before implementing them in the final documentation.

List of Questions:

1. Default questions (Question given as examples in the beginning):
   1. How does the node find other peers on the network?
   2. How is a new block or transaction received and validated?
   3. How is a transaction created and broadcasted?
2. Basics:
   1. Describe the primary purposes of a Bitcoin node.
   2. Why is it preferred to do transactions on your own node?
   3. Explain nodes and wallets.
   4. Try using bitcoind, bitcoin-cli, bitcoin-qt
3. Protocol - P2P:
   1. How does node find other peers?
   2. Explain the two ways in which a node can be connected with its peers.
4. Concurrency Model:
   1. Script Verification
      1. Explain how the number of threads used for script verification is decided. Explain the use of -par argument in it.
      2. Understand StartScriptCheckWorkerThreads(script_threads) function and try understanding how [PR #18710](bitcoin/bitcoin#18710) improved it.
   2. Send And Receive Messages To And From Peers
      1. Please explain how sending and receiving messages takes place multiple times, though it is wrapped by TraceThread(), which ensures unique execution.
5. Notification Mechanism:
   1. How to register a class to receive messages from the g_signals publisher?
   2. Does CMainSignals implement CValidationInterface? If now, then why do they have the same names for the methods?
   3. What is g_signals? Explain its structure and the process it employs to notify subscribers of the signal.

I would much appreciate feedback on this question to help me improve them.