dev branch work

[atticnotion]

Hi,

I built the latest dev branch and was able to successfully get the Armory UI running. It looks like the code is in the middle of migration to PySide2, and some of the menus are disconnected.

Most dialogs were originally implemented in one place (qtdialogs/qtdialogs.py, and some in qtdialogs/qtdefines.py), and some of these have been pulled out into their own modules. I successfully untangled a few more of them and got them to work properly in the UI, and then got stuck on ui/toolsDialogs.py. This imports from a module called jasvet.py, which was removed from the git repo in January. This module has some cryptographic functions and calls CppBlockUtils to get a random number using Crypto++ library.

What was the plan when this jasvet.py module was removed? Aside from that, am I on the right track in trying to untangle these PySide2 dialogs?

[goatpig]

CppBlockUtils to get a random number using Crypto++ library.

This was a C++ library wrapped around with SWIG. I got rid of that. Now there's a stand alone C++ process (CppBridge, I'll probably renamed it to ArmoryBridge in the near future) that handles all things crypto, manages wallets and talks to the blockchain service (ArmoryDB), and talks to the GUI via a 2 way encrypted socket.

All those Cpp.somefunction() calls are dead now. They have to be replaced with TheBridge.somefunction(), assuming the function name is transparent. They probably aren't, I'm trying to migrate all wallet handling and cryptopgraphic operations out of the python GUI and into the C++ bridge. The bridge is essentially a protobuf API service over an encrypted socket, and ArmoryQt is a "dumb" GUI in py3/qt5 (using pyside2).

Most dialogs were originally implemented in one place (qtdialogs/qtdialogs.py, and some in qtdialogs/qtdefines.py), and some of these have been pulled out into their own modules.

qtdialogs and qtdefines are waaaay too big. I'm trying to refactor all these dialogs into independant files and fix the circular import issues that is plaguing qtdialogs. I do it as I go: try some feature in the GUI, fix it and refactor its code into a dedicate file. Ideally, I want to isolate all of the legacy GUI this way. Then I can start adding new GUI in its own files using QtQuick (.qml).

jasvet.py, which was removed from the git repo in January. This module has some cryptographic functions and calls CppBlockUtils to get a random number using Crypto++ library.

This is old crypto code copied into the repo. It was used to sign and verify Bitcoin messages. It used python's own random library to generator the R value (instead of the C++ PRNG), so we stopped using it for signing circa 2014. It was kept around for verifying the signed Bitcoin messages however (since that needs no rng).

You can still find this file in the master branch: https://github.com/goatpig/BitcoinArmory/blob/master/jasvet.py

What was the plan when this jasvet.py module was removed?

I removed the module cause it was getting in the way when I started migrating to py3. I haven't gotten around to fixing this part of the GUI yet. It should be replaced by routines from BridgeSigner (https://github.com/goatpig/BitcoinArmory/blob/dev/armoryengine/CppBridge.py#L1128). The code exists in C++ but needs exposed via protobuf, so this one requires some C++ work as well.

Aside from that, am I on the right track in trying to untangle these PySide2 dialogs?

Yes. Feel free to PR your stuff, I'll gladly review and merge.

[atticnotion]

Ok. So what about CppBlockUtils.py, which no longer exists? There are a few calls into that module in dialog code (e.g. MultiSigDialogs.py calling CryptoECDSA()). Looks like the plan is to add boilerplate to CppBridge.py for each method like this.

Anyway so far I have been separating the dialogs and forms out incrementally, keeping the code in a runnable state as I make changes where possible, and not changing logic having to do with CppBridge code. But the calls to missing modules, where I'm not quite sure how to resolve them, are making it hard for me to proceed this way.

[goatpig]

Ok. So what about CppBlockUtils.py, which no longer exists? There are a few calls into that module in dialog code (e.g. MultiSigDialogs.py calling CryptoECDSA()).

CppBlockUtils.py was auto generated by SWIG to expose the C++ code. I don't use this anymore so all calls to CppBlockUtils in Python are now invalid and need to be replaced equivalents in CppBridge. They won't look quite the same since with the previous model, the py code would call cryptographic routines directly, whereas I've elected to isolate all of that in cpp binary now.

Most of this you're running into needs to be reworked to fit the new design, and in some cases the entire routine needs migrated to the cpp side still. I suggest you focus on those parts of the codebase that can work without that kind of heavy intervention and list those that require that. I'll get to them eventually.

[atticnotion]

I'd like to to create a PR of the work I've done, but I don't have an adequately anonymous github account and they've made it very difficult to create one.

Could I email you a patchset?

[goatpig]

Go ahead. It's gonna be pretty hard to back and forth on it though. Please wait till the end of the day however, so that I can push some changes I have locally, so that you can rebase on top of what I'm working against.

EDIT: new code is up.

[atticnotion]

I just sent the patch to the email address you use in git.

[goatpig]

Thanks for the patch! I made a branch with it, named attic_qtdialogs. Ill review the files and ask for changes as I go (if they're too big for a quick fix here and there). You can then contribute updates as stand alone patches. Once I'm happy with it, I'll merge the branch into dev. It may take me the whole week to review the change set, it's pretty big and I'm busy with a few other tasks atm.

[goatpig]

First remark, this may seem silly but it is what it is:

The Armory code uses 3 spaces per tabs instead of the usual 4. Your editor is set to use 4 so it auto rearranged a bunch of files (all the new files, as well as completely re-indenting MultisigDialogs.py and Wizard.py). Could you please stick to 3 spaces?

[atticnotion]

Ok. I am not using an IDE but ran the reindent tool from pypi, which only allows indent with 4 spaces for some reason.

Fixed them and am sending you a new patch.

[goatpig]

Thanks a lot, will get to it sometimes tomorrow, busy with another PR atm.

[goatpig]

I've gone over the change set, it's fairly straight forward. A few comments:

- The indent issue still exists in ui/MultiSigDialogs.py and ui/Wizards.py. Could you please fix those and submit only these changes as a new commit on top of the attic_qtdialogs branch? The rest of the work is good, we can start fixing the minor issues on a step by step basis.

- The patch introduced a lot of new and unnecessary white spaces, as well as deleting the new line at the end of several files, both of which are style infractions and tend to pollute changesets. Fortunately, git can fix a lot of this on its own (git am --whitespace=fix *path/to/patch*). It can't do anything about the missing new line at file end however, but can be addressed in a small commit. In the future, please try not to commit whitespace changes, possibly by having git ignore them when creating a patch.

- Apparently, I've fucked up with the license header in the segregated dialog files. It looks like I've elected to set the header to "Armory Technologies Inc 2011-2021" when I'm not allowed to do this (I'm not ATI), and it's wrong (I've done the work, whereas ATI is defunct). You've copied that license header along in your work and that made me realize it needs fixed. The license itself is also wrong, ATI uses AGPLv3, whereas I use MIT (more permissive). I do not have the right to loosen ATI's license, therefor the new code has to be copyrighted to me so that it can benefit from the looser license, hence the double license header which is the correct formulation. I'll fix those as I go, feel free to do the same.

- This syntax doesn't not carry over from PyQt4 to PySide2:

Code:

self.connect(self.btnAccept, SIGNAL("clicked()"), self.verifyUserInput)

The correct syntax is:

Code:

self.btnAccept.clicked.connect(self.verifyUserInput)

This stuff amounts for a lot of the syntax errors left.

------------------------

With regard to development, currently the following features are FUBAR and need a lot of attention. I don't intent to fix these in the near term:

- fragmented backups
- multisig/lockbox dialogs
- message signing

The following GUI elements are still broken but fairly easy to fix. I will likely give them attention:

- Wallet creation/restore wizard
- Changing the password on wallets
- Some minor display issues such as wallet labels, security mode, ownership

On top of that, there are obsolete features that are/will be replaced:

- The settings menu
- Wallet corruption checks and recovery

There is a set of new features that I want to add in the scope of the next release:

- A new "manager" dialog that will appear before the main lobby. It would have a few tabs with the purpose of walking the user through setting up Armory prior to start up. The tabs would be:

. Wallets: load all wallet files in the datadir, help with migration from the old to the new format, assist with unlocking public data (new wallet format encrypts all wallet data, public data can be locked with a separate password that requires a one time unlock at load)

. ArmoryDB: prepare all things related to setting up ArmoryDB. The new DB enforces AEAD, and supports both 1-way and 2-way auth. While local ArmoryDB requires path settings, connecting to a remote DB requires key exchange. This tab would assist with that.

. bitcoind: This tab will be succinct for now, just help with detecting bitcoind and custom pathing, possibly automate the process. Something to flesh out in the future.

- Transaction export format: There is a new format for passing transactions around now. This format is not backwards compatible with previous Armory versions. The new signer code also handles PSBT, so the idea is to allow (when eligible) for users to choose between formats when exporting signed/unsigned transaction blobs. The formats would be legacy, with a cutoff at 0.92, one at 0.96.x, PSBT and the new format.

- Support for paying to taproot addresses.

If you want to help with any of these, name the task and we can coordinate.

[atticnotion]

Looks like the reindent tool has some problems. Will send whitespaces fix patch. Will respond to your other comments after.

[atticnotion]

The reindent tool also deleted some continuation lines at the end of files, so I restored them.

Would like to help where I can. I'll let you know what I'm thinking after I've spent some more time using the dev branch build.

[goatpig]

Reviewed the full changeset, here are my comments:

######################
## review comments

ui/MultiSigDialogs.py:
    HighPrioTODO and some commented lines are in infraction of python indent
    rules. This code will not run as a result (intentional?)

qtdialogs/qtdefines.py:
    STRETCH as seen in qtdialogs.py has been redefined in qtdefines.py.
    Files importing STRETCH still look for it in qtdialogs.py (mostly). It
    should be imported from qtdefines.py only and STRETCH should be removed
    from qtdialogs.py (it makes more sense in qtdefines)

qtdialogs/ArmoryDialog.py:
qtdialogs/DlgHelpAbout.py:
qtdialogs/MsgBoxWithDNAA.py:
    Hardcoded path to images in the root img folder is invalid
    (i.e ./img/MsgBox_info48.pgn). These paths are probably invalid now that
    the files live one folder deeper (should be ../img instead, I suspect)

qtdialogs/MsgBoxCustom.py:
    Same image pathing issue as with MsgBoxWithDNAA.
    Also, the file uses the old connect/SIGNAL syntax which is invalid in
    pyside2, e.g:

        self.connect(btnOk, SIGNAL('clicked()'), self.accept)

    it should be

        btnOk.clicked.connect(self.accept)

qtdialogs/DlgWltRecoverWallet.py:
qtdialogs/DlgCorruptWallet.py:
    These dialogs are deprecated, new wallets do not make use of it. Shouldn't
    spend time fixing these. There is no cause to remove them however, they're
    not imported anywhere. May revive this stuff later but the whole functionality
    is obsolete due to how the new wallet files work: they're fully encrypted,
    with MACs, so file corruptions will always be detected natively instead of
    potentially altering wallet data silently.

qtdialogs/DlgSettings.py:
    This dialog is super broken and will take a lot of work to fix besides
    the pyside2 migration stuff. I'm thinking of removing those features in
    there that are broken and reimplementing them in the "manager" dialog
    instead of fixing them in place. Shouldn't spend time on this code as
    a result.

qtdialogs/RestoreWOData.py:
    This dialog requires a decent amount of time to work with both Armory
    legacy wallets and BIP32 wallets. The new wallet format also allows for
    exporting the wallet structure and meta data (all data that isn't private
    keys), as well as encrypt that data for transit, so all things related to
    WO and meta data import/export will need a UX rework.

qtdialogs/DlgRestoreSingle.py:
    The stuff that applies to RestoreWOData applies to wallet restore in
    general. Put another way, time shouldn't be spent trying to simply fix
    these dialogs, that effort will be lost when reworking the restore
    dialogs to fit the new wallet functionalities.

qtdialogs/DlgRestoreFragged.py:
    Same as above. The SSS code is dead atm anyways so this feature is nowhere
    near funcitonal.

qtdialogs/DlgPasswd3.py:
    Invalid img path.
    Invalid connect/SIGNAL syntax.

qtdialogs/DlgInflatedQR.py:
    Missing a few imports.

dynamicImport.py:
    Depreacted file used to load plugins, a feature that was never really
    fleshed out. Ignore.

jasvet.py:
    Python implementation of the Bitcoin message signing protocol. Unsafe and
    deprecated for signing, only used for verifying signatures. Should be fully
    replaced with calls to CppBridge.

pytest/*.py:
    The python tests are deprecated. A few of them can be revived but over
    95% of what they cover has been moved to CppBridge. Coverage only
    really make sense for CppBridge now. The python client is now a dumb UI
    that does not implement any core logic, only GUI elements. CppBridge is
    an API process that communicates via protobuf, so it's pretty easy to
    flesh out a headless test suite for it instead.

qtdialogs/.pylintrc
    This looks like some sort of linter dependency and/or a syntax helper.
    These kind of files shouldn't be committed.

ui/toolsDialogs.py:
    Minor indent infractions in the import section.

######################
## files with nothing to comment on:
ui/QrCodeMatrix.py
ui/Wizards.py
ui/CoinControlUI.py
ui/FeeSelectUI.py
ui/TxFramesOffline.py
qtdialogs/DlgWalletSelect.py
qtdialogs/DlgWalletDetails.py
qtdialogs/DlgUnlockWallet.py
qtdialogs/DlgShowKeyList.py
qtdialogs/DlgSetComment.py
qtdialogs/DlgSendBitcoins.py
qtdialogs/DlgRequestPayment.py
qtdialogs/DlgReplaceWallet.py
qtdialogs/DlgQRCodeDisplay.py
qtdialogs/DlgProgress.py
qtdialogs/DlgOfflineTx.py
qtdialogs/DlgNewAddress.py
qtdialogs/DlgMigrateWallet.py
qtdialogs/DlgKeypoolSettings.py
qtdialogs/DlgIntroMessage.py
qtdialogs/DlgExportTxHistory.py
qtdialogs/DlgEULA.py
qtdialogs/DlgDispTxInfo.py
qtdialogs/DlgConfirmSend.py
qtdialogs/DlgChangePassphrase.py
qtdialogs/DlgBrowserWarn.py
qtdialogs/DlgAddressBook.py
ArmoryQt.py
armorymodels.py
armoryengine/ArmoryUtils.py

I'm considering merging this stuff into the dev branch and moving to a file by file or feature by feature basis past this point. What do you think? Do you want to cleanup this PR some more or move on?

[atticnotion]

I will try to clean this up a little more based on your comments, then after this can try to keep my commits more granular. This changeset became too unwieldy as it got bigger.

[goatpig]

Works for me. Thanks for the help.

[Carlton Banks]

ok, so I finally have the dev branch in some kind of working state (real life + building problems intervened before)

one problem persists, namely that I cannot get the CppBridge to launch ArmoryDB in latest dev branch. No clue whether this is because it's broken or just that I did something wrong. ArmoryDB works fine on it's own, but launching ArmoryQt.py once ArmoryDB is started/synced does not create the bridge connection. Yes, I re-ran the c20p1305_cffi.py script. And also make clean. Not in that order

[goatpig]

ok, so I finally have the dev branch in some kind of working state (real life + building problems intervened before)

one problem persists, namely that I cannot get the CppBridge to launch ArmoryDB in latest dev branch. No clue whether this is because it's broken or just that I did something wrong. ArmoryDB works fine on it's own, but launching ArmoryQt.py once ArmoryDB is started/synced does not create the bridge connection. Yes, I re-ran the c20p1305_cffi.py script. And also make clean. Not in that order

Try running ArmoryDB with --public

[Carlton Banks]

yep, that worked

I created a new wallet. I can't see how it makes sense to scan the blocks for transactions in that case, but that happened, and all was well. Sadly, there was no surprise BTC

It seems there are methods in cppforswig/BridgeAPI/WalletManager to convert the 0.96.x format to the new one? Can I call them somehow, or is there no userspace interface yet?