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

pyln-testing: require bitcoin v0.20.1 for PSBT handling, and create wallet for v0.21 #4179

Merged

Conversation

rustyrussell
Copy link
Contributor

With older bitcoind, PSBTs fail:

    def test_utxopsbt(node_factory, bitcoind, chainparams):
...
>       psbt = bitcoind.rpc.decodepsbt(funding['psbt'])

tests/test_wallet.py:561: 
...
self = <bitcoin.rpc.RawProxy object at 0x7f4ec602e100>, service_name = 'decodepsbt'
args = ('cHNidP8BADMCAAAAAaoMihSVXlpdBHGcJePiroqtwq/b1zu09j8IkTG4OKs7AQAAAAD9////AGYAAAAAAQDeAgAAAAABAefqB6BkZE1/AqXaf36T02a7.../7Stf971PEgvUXgvASECXPTIO6tIVxDih6tfKy6suj6WJhhjycwoaTeuso/AQ8llAAAAAQEfQEIPAAAAAAAWABQB+tkKvNZml+JZIWRyLeSpXr7hZQA=',)
postdata = '{"version": "1.1", "method": "decodepsbt", "params": ["cHNidP8BADMCAAAAAaoMihSVXlpdBHGcJePiroqtwq/b1zu09j8IkTG4OKs7AQ...gvUXgvASECXPTIO6tIVxDih6tfKy6suj6WJhhjycwoaTeuso/AQ8llAAAAAQEfQEIPAAAAAAAWABQB+tkKvNZml+JZIWRyLeSpXr7hZQA="], "id": 1}'
headers = {'Authorization': b'Basic cnBjdXNlcjpycGNwYXNz', 'Content-type': 'application/json', 'Host': 'localhost', 'User-Agent': 'AuthServiceProxy/0.1'}
response = {'error': {'code': -22, 'message': 'TX decode failed PSBT is not sane.: iostream error'}, 'id': 1, 'result': None}

But with bitcoind v0.21 (or at least, current master), we fail every test with:

    @pytest.fixture
    def bitcoind(directory, teardown_checks):
        chaind = network_daemons[env('TEST_NETWORK', 'regtest')]
        bitcoind = chaind(bitcoin_dir=directory)
    
        try:
            bitcoind.start()
        except Exception:
            bitcoind.stop()
            raise
    
        info = bitcoind.rpc.getnetworkinfo()
    
        if info['version'] < 160000:
            bitcoind.rpc.stop()
            raise ValueError("bitcoind is too old. At least version 16000 (v0.16.0)"
                             " is needed, current version is {}".format(info['version']))
    
        info = bitcoind.rpc.getblockchaininfo()
        # Make sure we have some spendable funds
        if info['blocks'] < 101:
>           bitcoind.generate_block(101 - info['blocks'])

contrib/pyln-testing/pyln/testing/fixtures.py:138: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
contrib/pyln-testing/pyln/testing/utils.py:397: in generate_block
    return self.rpc.generatetoaddress(numblocks, self.rpc.getnewaddress())
contrib/pyln-testing/pyln/testing/utils.py:320: in f
    return proxy._call(name, *args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <bitcoin.rpc.RawProxy object at 0x7f59352554f0>, service_name = 'getnewaddress', args = (), postdata = '{"version": "1.1", "method": "getnewaddress", "params": [], "id": 1}'
headers = {'Authorization': b'Basic cnBjdXNlcjpycGNwYXNz', 'Content-type': 'application/json', 'Host': 'localhost', 'User-Agent': 'AuthServiceProxy/0.1'}
response = {'error': {'code': -18, 'message': 'No wallet is loaded. Load a wallet using loadwallet or create a new one with createwallet. (Note: A default wallet is no longer automatically created)'}, 'id': 1, 'result': None}

Signed-off-by: Rusty Russell [email protected]
Changelog-Changed: *** Requires bitcoind v0.20 or above ***

@rustyrussell rustyrussell added this to the v0.9.2 milestone Nov 4, 2020
@rustyrussell rustyrussell changed the title pyln-testing: require bitcoin v0.20 for PSBT handling, and create wallet for v0.21 pyln-testing: require bitcoin v0.20.1 for PSBT handling, and create wallet for v0.21 Nov 4, 2020
Copy link
Collaborator

@darosior darosior left a comment

Choose a reason for hiding this comment

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

ACK 90f4ea6

Had the same commit here 8e9ea4f but without the 20100 requirement :).
Would be great to bump to 0.21 when it's released and use Sqlite descriptors wallets, in my experience they are much faster and would probably speed up the entire test suite.

@darosior
Copy link
Collaborator

darosior commented Nov 4, 2020

Oh, Liquid is still on 0.18.X (travis)..

@niftynei
Copy link
Collaborator

niftynei commented Nov 4, 2020

I've flagged this as an issue with the elements team a while back. They're in the process of updating the codebase to match/harmonize with bitcoind 0.21 and should have a new/compatible version released by the end of the month 🤞

The most up-to-date version of elementsd available is 0.18.0 (which is what travis has).

@niftynei
Copy link
Collaborator

niftynei commented Nov 6, 2020

Closes #4144

@cdecker
Copy link
Member

cdecker commented Nov 6, 2020

Hm, seems like it changed a number of times recently. See #4088 for yet another interface change that failed some tests.

@niftynei
Copy link
Collaborator

niftynei commented Nov 6, 2020

Pushed up a patch up to handle special case for liquid-regtest versions.

@rustyrussell
Copy link
Contributor Author

Ack 2577219

@jsarenik
Copy link
Collaborator

jsarenik commented Nov 8, 2020

Just a note: Yes there are two ways to do it. See #4088 with all the links to bitcoin/bitcoin issues. Namely bitcoin/bitcoin#20034.

Copy link
Collaborator

@jsarenik jsarenik left a comment

Choose a reason for hiding this comment

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

Only one of #4088 or this change should stay. If both are in the code, they may shoot someone in the foot later.

# At 0.21, createwallet needed, but in 0.20.1 createwallet then broke
# calls to getnewaddress with "bitcoin.rpc.JSONRPCError: {'code': -19, 'message': 'Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).'}"
if info['version'] > 200100:
bitcoind.rpc.createwallet("test")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is exactly how I tried to fix it first in #4088, but in the end I used the -wallet="default" option in contrib/pyln-testing/pyln/testing/utils.py because it is backwards compatible with at least 0.20.1. @rustyrussell please consider which should stay.

@jsarenik
Copy link
Collaborator

jsarenik commented Nov 8, 2020

…let for v0.21

With older bitcoind, PSBTs fail:

```
    def test_utxopsbt(node_factory, bitcoind, chainparams):
...
>       psbt = bitcoind.rpc.decodepsbt(funding['psbt'])

tests/test_wallet.py:561:
...
self = <bitcoin.rpc.RawProxy object at 0x7f4ec602e100>, service_name = 'decodepsbt'
args = ('cHNidP8BADMCAAAAAaoMihSVXlpdBHGcJePiroqtwq/b1zu09j8IkTG4OKs7AQAAAAD9////AGYAAAAAAQDeAgAAAAABAefqB6BkZE1/AqXaf36T02a7.../7Stf971PEgvUXgvASECXPTIO6tIVxDih6tfKy6suj6WJhhjycwoaTeuso/AQ8llAAAAAQEfQEIPAAAAAAAWABQB+tkKvNZml+JZIWRyLeSpXr7hZQA=',)
postdata = '{"version": "1.1", "method": "decodepsbt", "params": ["cHNidP8BADMCAAAAAaoMihSVXlpdBHGcJePiroqtwq/b1zu09j8IkTG4OKs7AQ...gvUXgvASECXPTIO6tIVxDih6tfKy6suj6WJhhjycwoaTeuso/AQ8llAAAAAQEfQEIPAAAAAAAWABQB+tkKvNZml+JZIWRyLeSpXr7hZQA="], "id": 1}'
headers = {'Authorization': b'Basic cnBjdXNlcjpycGNwYXNz', 'Content-type': 'application/json', 'Host': 'localhost', 'User-Agent': 'AuthServiceProxy/0.1'}
response = {'error': {'code': -22, 'message': 'TX decode failed PSBT is not sane.: iostream error'}, 'id': 1, 'result': None}
```

Signed-off-by: Rusty Russell <[email protected]>
Changelog-Changed: *** Requires bitcoind v0.20.1 or above ***
@niftynei
Copy link
Collaborator

niftynei commented Nov 9, 2020

Updated to remove duplicated fix (covered in 8e9ea4f), squash some stuff to make it make more sense.

@niftynei
Copy link
Collaborator

niftynei commented Nov 9, 2020

ACK e913f19

@niftynei niftynei merged commit fa006fd into ElementsProject:master Nov 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants