Allow for custom ports to be specified for bitcoin RPC

[hkrugersa]

It looks like currently the Bitcoin's RPC ports are hard-coded to the defaults. I see that issue #49 addressed this by providing the capability for custom LND ports, but it seems that the Bitcoin ports were overseen and always refer to the hard-coded values.

This should be relatively easy to fix by just checking the relevant configuration file for the rpcport value. However, I see from version 0.17.0 onwards the configuration file supports that you can specify parameters for mainnet, testnet and regtest all in the same file with a section tag like [main] or [test] or [regtest].

The following section from the release documentation is of interest:(https://bitcoin.org/en/release/v0.17.0#configuration-sections-for-testnet-and-regtest)

If the following options are not in a section, they will only apply to mainnet: addnode=, connect=, port=, bind=, rpcport=, rpcbind= and wallet=. The options to choose a network (regtest= and testnet=) must be specified outside of sections.

Thus if the section isn't specified in the file those options will be ignored for testnet and regtest. Hence, it is necessary to populate the [test] section tag at the beginning of the testnet configuration file for bitcoin version 0.17.0 onwards.

I can have a look at this and submit a pull request once I'm done, but I need to know whether the assumption can be made that the bitcoin node will always be the newest version (or at least > 0.17.0) or should the configuration file class check the bitcoin version before inserting the [test] or [main] section tag?

[hkrugersa]

I've forked the code and made the relevant changes, which can be viewed at

https://github.com/PierreRochard/node-launcher/compare/master...hkrugersa:master

Please let me know if there are any problems or if you require any additional changes to be made.

In the configuration_file.py I've had to pass in a network keyword argument for the class to know whether it is for mainnet or testnet. It is a bit ugly, so please let me know if you're happy with that or if you have any other ideas of how to approach it.

I've also noticed that in the PortsLayout widget, the application needs a restart for the ports to refresh (if changed in the text file midway). I'm not sure why that would be, but I'm guessing this is due to the fact that the variables are only set on initialization of the widget and when you click to close the advanced window it doesn't actually close it, but merely hides it. I'll have a look if I can find a way to fix this, but please let me know if you have any ideas.

[PierreRochard]

Thank you @hkrugersa! This is a great improvement!

Changing the PortsLayout widget should be similar to how the Data Directory widget changes when a new directory is selected, so take a look at that!

Unfortunately I won't be able to dig into this until Friday, happy hacking!

[hkrugersa]

Thanks Pierre!

Just an update, I'm almost done with the changes. Just updating the tests and doing some final testing. I decided to use the QFileSystemWatcher object to monitor the relevant files in the Configurationfile object. This way you can connect the signals to all relevant objects that need to be notified if the lnd or bitcoin config files changed. It also means you can change the file outside of the application without going through the advanced widget and it would still work.

Hopefully I'll be able to finish up this evening still, but I'll keep you updated.

[PierreRochard]

That is really awesome @hkrugersa thank you! Huge improvement

[hkrugersa]

Hi @PierreRochard,

Apologies for the delays, but have been trying to catch up with all the changes you guys have been making to the codebase :-) Well done on all the work by the way.

Just an update from my side, I'm pretty much done with the code on my end - but would like to ask a few questions before I submit the pull request to make sure I've covered everything:

• I initially planned to only include support for custom ports, but it dawned on me that once you allow the user to edit the configuration file you open up a can of worms of him changing a bunch of other things that also need to be considered. I've written up functions in the bitcoin and lnd classes that will inform the user that a restart is required if any of the key parameters change in the configuration file. A snapshot of the configuration is stored as a dict in each class upon launch and every time the configuration file changes it is compared to the new snapshot. At the moment the alert to restart the nodes is only given on the advanced_widget (due to the issue mostly related to ports info), but I think it would be useful to include some sort of notification on the main_widget or network_widget or alternatively as a messagebox popup. What do you think would be best for this? Also, initially I catered for only the ports - but if a user starts changing other things - it might be a good idea to just alert him that a restart is required and to also update the relevant functions in the rest of the application. Also, I think it would be a good idea if something is found that constitutes an invalid configuration file. This is a fair bit of scope creep, but I think it would be useful for the UX. Would you be comfortable that I include that in the pull request or should I only submit what I have on the ports? I'll submit my thoughts in the next post of what should constitute an invalid configuration file. But please let me know if you have any thoughts as well.

• Another feature that I think would be useful is to support the rpcauth parameter instead of the rpcuser and rpcpassword. Especially if the disablewallet function is to be removed (which I saw is now possible right?). This way the rpc password is not stored in plaintext, but in hashed format according to my understanding. I can also try and add support for this in my pull request, but if you'd like me to raise that as a seperate issue and tackle it seperately (or at all) - please let me know.

• Finally, I've tried to add a few unit tests as well for the code - but since the configuration file uses the QFileSystemWatcher object from Qt it requires a Qt Application to connect the signals emitted to the slots I've specified in the bitcoin, lnd and ports_layout classes. Is there a way you know of to test these without launching the whole program?

[hkrugersa]

My ideas for what constitutes an invalid configuration file for bitcoin:

• Unsupported name_tags
• Lines without "=" statements, except for section tags (although not sure if bitcoin ignores these by default)
• txindex=0 (I'm assuming this would defeat the purpose of what node-launcher is meant for)
• server=0 (same as above)
• No rpcuser statement
• No rpcpassword statement (or alternatively no rpcauth statement)
• A timeout statement below some threshold (tbd)

[hkrugersa]

I'm a bit in the dark as to all the lnd configurations, but I'll give it a shot as to what could constitute an invalid lnd configuration file:

• bitcoin.node=btcd option
• section tags for btcd and any options related to btcd (i.e. btcd.rpchost, btcd.rpcuser etc)
• No bitcoin details
• Any litecoin shitcoinery :-)

[hkrugersa]

Additional thoughts:

• Checking testnet settings in app vs configuration file

[hkrugersa]

For first pass at critical bitcoin configuration file changes that require restart (that the user will be notified of) include the following:

• rpcuser
• rpcpassword
• rpcport
• port
• disablewallet
• datadir
• zmqpubrawblock
• zmqpubrawtx
• prune
• txindex
• timeout

[hkrugersa]

Also my basic rules are that anything that requires a bitcoin restart used by lnd, requires a lnd restart

[PierreRochard]

Thank you @hkrugersa! Please submit a pull request so I can comment on the code, happy to help with that process or ping Justin!

[PierreRochard]

• txindex=0 (I'm assuming this would defeat the purpose of what node-launcher is meant for)

txindex is disabled if the user is pruning, and LND is compatible with no txindex, it's just a little slower