Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bitcoin full node support #59

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from
Draft

Bitcoin full node support #59

wants to merge 37 commits into from

Conversation

AnotherDroog
Copy link
Member

@AnotherDroog AnotherDroog commented Sep 26, 2019

This adds a pruned bitcoind with low resource limits.

Intended to provide invoicer with mempool capabilities and block verification.

Please note that pruned mode will sync from genesis unless a recent chain snapshot is applied.

TODO:

  • Generate rpcauth for bitcoin.conf
  • Automatically populate invoicer.conf with credentials

@AnotherDroog AnotherDroog added the enhancement New feature or request label Sep 26, 2019
@AnotherDroog AnotherDroog self-assigned this Sep 26, 2019
@nolim1t
Copy link
Member

nolim1t commented Sep 26, 2019

Could we have the option of an actual full node as well as a pruned full node?

@AnotherDroog
Copy link
Member Author

Could we have the option of an actual full node as well as a pruned full node?

@nolim1t Yes of course, the only change necessary is prune=0 in bitcoin.conf

There could be another compose folder without neutrino, where lnd relies only on bitcoind.

PRs welcome!

@AnotherDroog
Copy link
Member Author

  • Added parsing and validation for ini and toml config files.
  • rpcauth is now generated automatically for bitcoind and invoicer

@AnotherDroog
Copy link
Member Author

Expected output:

sudo noma start
✅ media directory exists
✅ noma directory exists
✅ compose directory exists
✅ bitcoind directory exists
✅ bitcoind config file exists
✅ Set rpcauth successfully
✅ lnd directory exists
✅ lnd.conf exists
Creating fullnode_bitcoind_1 ... done
Creating fullnode_lnd_1      ... done
Creating fullnode_invoicer_1 ... done

@@ -25,9 +25,9 @@ jobs:
run: |
pip install flake8
# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
flake8 *.py noma/*.py tests/*.py --count --select=E9,F63,F7,F82 --show-source --statistics
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the --select=???? things? Could you tl;dr in a comment above?

compose/fullnode/docker-compose.yml Show resolved Hide resolved

[Routing]
; Save bandwidth by only downloading minimum channel filters necessary
routing.assumechanvalid=1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the thing that caused the most recent lnd CVE?

I don't know. Just asking now.

; Not using bitcoind as backend by default due to pruning limitations
[Bitcoin]
bitcoin.active=1
bitcoin.testnet=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No point in having testnet line here.

@@ -3,8 +3,8 @@
setup(
name="noma",
version="0.5.1",
packages=["noma"],
install_requires=["psutil", "docopt", "requests", "docker-compose"],
packages=["noma", "configobj"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's that? Why does it add 2k LOC to the repo here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to make some changes to the way the library writes out ini files to conform to bitcoind and lnd style.

I'll submit a PR upstream, once it's merged and released configobj/ can be removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, just add a comment saying that somewhere :).

Ideally with a link to an issue tracking it.

noma/bitcoind.py Show resolved Hide resolved

bitcoind_conf = "/media/archive/archive/bitcoin/bitcoin.conf"
bitcoind_conf_exists = pathlib.Path(bitcoind_conf).is_file()
raise IOError("❌ bitcoin directory missing")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not cfg.BITCOIN_PATH.is_dir():
	raise IOError("❌ bitcoin directory missing")

print("✅ bitcoind directory exists")

noma/bitcoind.py Outdated
if section:
return config[section][key]
try:
value = config[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is try/except is the only way to check if a key exists?

Plus, no check if key[section] exists above.

noma/bitcoind.py Outdated Show resolved Hide resolved
noma/bitcoind.py Outdated
except KeyError:
print("rpcauth unset")

if rpcauth_val == '': # only change if value is unset
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better when "the happy path" is left most ;).

So flipping this if might improve clarity.

if some-error:
	print
	return

back to happy path ^o^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants