make Python bindings externally usable

jamesob commented at 4:48 pm on November 8, 2021: member

Summary: make very useful test_framework Python bindings reusable from outside.

Recently I’ve been interested in approaches to implement alternate P2P transports, with some prior efforts being

Facilitating a diversity of both transports and semantics in the peer-to-peer layer seems like a worthwhile effort. At the same time, I’ve grown wary of undertaking projects that would expand the scope of what Core does.

These two ideas have led me to think that one interesting avenue for doing alternative P2P networking is by running a relay program locally alongside bitcoind that can adapt whatever networking logic and protocol the user desires into bitcoin P2P. In conjunction with -noconnect/-addnode and such a relay, you could have a Bitcoin node that exclusively speaks an alternate network externally without any changes to Core.

Anyway, this is all a roundabout preamble for this fairly modest change, which makes the (very useful and well-maintained) Python utilities we have for interacting with bitcoind pip installable and so usable as a library by other programs. A lot of effort has been put into things like P2P message (de)serialization and RPC communication and it would be nice to have these inherently well-maintained utilities reusable from the outside.

This is a small proof-of-concept patch that can be more polished if necessary, but as written it makes the test_framework infrastructure more generally useful.

I intend on using this library to build some workable examples of adapting DNS and radio to Bitcoin’s P2P communications.

Even beyond alternate P2P designs, these tools would be very handy to be able to reuse; e.g. bitcoinperf rewrites a lot of node management logic. It would be preferable to install Python dependencies from this repo, which is already trusted and maintained, than to have to copy/paste vendor code or go to third parties.

jamesob force-pushed on Nov 8, 2021

laanwj commented at 5:06 pm on November 8, 2021: member

I’m sorry but I’m not sure this is a good idea. Having the test framework be internal makes it very flexible, easy to adopt to our changing needs. Offering it as an external API means having to deal with versioning, backwards compatibility, external feature requests, and so on.

Aren’t there other Python bitcoin frameworks that could do what you want?

It would be preferable to install Python dependencies from this repo, which is already trusted and maintained, than to have to copy/paste vendor code or go to third parties.

I really dislike this reasoning. Yes, maybe you trust this repo, but we’ve always resisted the centralizing force of having everything come from a single project and maintained under a single banner. I think this is one of bitcoin’s stroing points compared to other coin ecosystems.

test: move Python bitcoin utilities into a pip-installable module

Sort of move-onlyish.

93d3022c4c

test: make bitcoincore-local imports relative 2df52389b5

test: backwards compat with test suite 5c8a9cfe28

test: add logger example ce33369c76

jamesob force-pushed on Nov 8, 2021

jamesob commented at 5:10 pm on November 8, 2021: member

Aren’t there other Python bitcoin frameworks that could do what you want?

I think offering this with the caveat that there are essentially no API compatibility guarantees would be much better than nothing. I think an equivalent Python library would basically just be a copy/paste of much of the content in test_framework but would be more prone to bit-rot.

The code here is always functional by nature of it being used to test bitcoin, and there’s huge value in that. IMO users should be able to use this code externally if they accept the caveat that API compatibility is not a consideration.

laanwj commented at 5:15 pm on November 8, 2021: member

Yes, I do this myself too. Either I symlink test_framework or copy the necessary modules into my project (usually authproxy.py). I think that can be super useful. But I don’t want to commit to this. I don’t think it’s a good idea to support this outside the immediate developer community of this project, which e.g. making it installable with pip would suggest.

Also remember that this is a test framework. It is not to be used for production usage of any kind. E.g.: please don’t use it with real money at stake.

jamesob commented at 5:17 pm on November 8, 2021: member

I really dislike this reasoning. Yes, maybe you trust this repo, but we’ve always resisted the centralizing force of having everything come from a single project and maintained under a single banner.

I understand where you’re coming from here, but if this is code that we already have to maintain anyway, why not make it usable by others? The maintenance and review cycle in Core is incredibly thorough and well-attended - I am not a fan of expanding it to things that we don’t already work on, but if this code is already under the umbrella anyway, it is a tremendous benefit to be able to leverage it.

I don’t think it’s a good idea to support this outside the immediate developer community of this project, which e.g. making it installable with pip would suggest.

Note that I intentionally omitted mentioning, say, PyPI for this reason.

sipa commented at 5:18 pm on November 8, 2021: member

I think the test framework codebase has a common ancestor with https://github.com/petertodd/python-bitcoinlib; perhaps to some extent that is usable too?

IMO users should be able to use this code externally if they accept the caveat that API compatibility is not a consideration.

Experience shows that this just doesn’t work. If it’s usable, people will use it, and then complain about changes that break compatibility.

test: add README for bitcoincore Python module fedb35f2ce

jamesob force-pushed on Nov 8, 2021

jamesob commented at 5:53 pm on November 8, 2021: member

E.g.: please don’t use it with real money at stake.

I agree with this general caveat, but I would much rather rely on this repository’s review process for money-sensitive operations than some additional third party if possible.

Experience shows that this just doesn’t work. If it’s usable, people will use it, and then complain about changes that break compatibility.

I think having a strong caveat in the README that

(i) makes it clear that API compatibility will not be given consideration and
(ii) recommends pinning a specific hash of the bitcoincore library

would absolve our process of any practical hurdles. There is an easy-to-articulate process for freezing this dep and manually backporting in updates that we could outline with little/no cost to our development cycle.

laanwj commented at 5:56 pm on November 8, 2021: member

but I would much rather rely on this repository’s review process for money-sensitive operations than some additional third party if possible.

We do not review the test code with this scenario in mind.

DrahtBot added the label Docs on Nov 8, 2021

DrahtBot added the label Tests on Nov 8, 2021

josibake commented at 6:11 pm on November 8, 2021: member

the (very useful and well-maintained) Python utilities we have for interacting with bitcoind

I don’t think this is a fair statement. The test framework python code is not meant to be utilities for interacting with bitcoind. It’s sole purpose is to be the functional test framework. While there is likely some overlapping use-case, I think the test framework code should be written and optimized to efficiently run the test framework only. If people did start using this as a more general-purpose library, I’d be worried that decisions to make it more generally useful might degrade its performance/usefulness in the test framework.

Secondly, from personal experience, I did repurpose this code for a personal project I was doing and found myself fighting the code a lot and hacking it to make it more generally useful. I ended up using https://ofek.dev/bit/ , which I found to be much easier to use.

ghost commented at 6:35 pm on November 8, 2021: none

which makes the (very useful and well-maintained) Python utilities we have for interacting with bitcoind pip installable and so usable as a library by other programs

This is interesting and if I understand this correctly it can be used in a project like https://github.com/prayank23/bitcoin-ps

A GUI application to test Bitcoin Core which has options similar to https://portswigger.net/burp:

Python scripts
Different options for fuzzing
Setup regtest nodes with few clicks

If I had to create such application using C# maybe I can use Iron Python

Or maybe someone can use flask to create a web UI for testing Bitcoin Core, code playground etc.

JeremyRubin commented at 6:49 pm on November 8, 2021: contributor

one thing i’ve hit up against is that certain things (e.g. in messages.py) are missing the APIs you want for building stuff and when I did sapio in python (somewhere in old commits of https://github.com/sapio-lang/sapio) I had to patch a number of things iirc when I vendored the lib out of core.

A benefit of making it a library is that these patches/changes might be more likely to get upstreamed which could benefit test writers too.

Maybe a good intermediate would be to just make a workflow of some kind that triggers a release of whatever the latest python code is every time there is a core release? This can happen without the release being core official.

jamesob commented at 6:49 pm on November 8, 2021: member

I don’t think this is a fair statement.

You don’t think it’s a fair statement that the Python code in the test framework is “very useful and well-maintained”? Or that it helps facilitate interaction with bitcoind?

If people did start using this as a more general-purpose library, I’d be worried that decisions to make it more generally useful might degrade its performance/usefulness in the test framework.

I have mentioned numerous times above that strong caveats could be included to make it clear that this library is driven around test usage for Bitcoin Core vs. anything else.

We do not review the test code with this scenario in mind.

This is a good point and I sympathize with it, but the reality is that because of this code’s use and inclusion in this repository, it is probably better maintained than alternatives by an order of magnitude. I support making a big disclaimer that “this library is not intended for use with real funds” but that doesn’t change the reality that de facto I would still probably choose to rely on this code over every usable alternative given its heavy use and constant review.

jamesob commented at 6:50 pm on November 8, 2021: member

Maybe a good intermediate would be to just make a workflow of some kind that triggers a release of whatever the latest python code is every time there is a core release? This can happen without the release being core official.

Yup, this is what I intend to do if this change is not accepted. But then users will sadly have to trust/verify that I have not tampered with the code instead of just relying on the existing processes here.

DrahtBot commented at 5:28 am on November 9, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23371 (test: MiniWallet: add P2TR support and use it per default by theStack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke commented at 8:41 am on November 9, 2021: member

Not sure what the goal here is, but the ones mentioned in the OP seem completely different scopes. I think writing an alternative/exclusive transport layer with the test code should not be done. The test structs are not guaranteed to be compatible with the p2p layer. They are only guaranteed to be compatible with the Bitcoin Core serialization at one specific commit. I think this should also be our goal, but there seems to be disagreement (https://github.com/bitcoin/bitcoin/pull/21357#discussion_r600727982)

Also, as mentioned previously private keys in the tests are treated like a 123456 password. Not something that should be used outside of tests.

If the goal is to make it easier to write external Bitcoin Core tests, then that seems ok. Do you need pip for that or can you use the existing TestShell for it? https://github.com/bitcoin/bitcoin/tree/master/test/functional#prototyping-tests

MarcoFalke commented at 9:15 am on November 9, 2021: member

And if TestShell isn’t sufficient, can’t you use this:

https://github.com/bitcoin/bitcoin/blob/94db963de501e4aba6e5d8150a01ceb85753dee1/contrib/message-capture/message-capture-parser.py#L16

josibake commented at 9:46 am on November 9, 2021: member

You don’t think it’s a fair statement that the Python code in the test framework is “very useful and well-maintained”? Or that it helps facilitate interaction with bitcoind?

Sorry, should have been more clear: I think it is incorrect to think of this code as “well-maintained utilities for interacting with bitcoind.” This code is for the functional testing framework and it is written and reviewed with only that purpose in mind.

MarcoFalke added the label Brainstorming on Nov 9, 2021

MarcoFalke removed the label Docs on Nov 9, 2021

DrahtBot added the label Needs rebase on Nov 9, 2021

DrahtBot commented at 10:40 am on November 9, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

MarcoFalke commented at 12:38 pm on November 9, 2021: member

Also, as mentioned previously private keys in the tests are treated like a 123456 password. Not something that should be used outside of tests.

Not to mention that authproxy itself is broken. Repeating RPCs until they succeed, ignoring error conditions.

jamesob commented at 2:58 pm on November 9, 2021: member

The test structs are not guaranteed to be compatible with the p2p layer. They are only guaranteed to be compatible with the Bitcoin Core serialization at one specific commit. I think this should also be our goal, but there seems to be disagreement (#21357 (comment))

This seems counter-intuitive, since I would have thought that one of the central purposes of the functional test framework is to serve as external verification that there are not unexpected changes to P2P semantics/serialization. Allowing that the P2P client in the functional tests is overtly coupled to a single Bitcoin Core commit (and not the more general P2P protocol) seems like an unfortunate concession.

Perhaps you’re right that the current test_framework code is too muddled with test-specific stuff to be generally useful. Admittedly I haven’t repurposed much aside from test_framework.{p2p,messages}, neither of which seems to have much test-specific content. Maybe it would be worth outlining those places where there are “123456 passwords” and isolating them to a specific module, provided such a thing is not overly cumbersome to do.

Anyway, I guess I’ll close this and either do Yet Another Third Party Python Module for my own purposes or think more about an alternate approach - e.g. somehow uniting src/protocol.* and components like net::CSerializedNetMsg into a shared library that can be used outside of Bitcoin Core and wrapped in $whatever_lang.

jamesob closed this on Nov 9, 2021

sipa commented at 3:00 pm on November 9, 2021: member

@jamesob Have you looked at https://github.com/petertodd/python-bitcoinlib? It also has P2P serialization etc.

ajtowns commented at 3:00 pm on November 9, 2021: member

Aren’t there other Python bitcoin frameworks that could do what you want?

I’ve considered proposing a PR like this before too; for testing/research the test_framework API is really handy – it has a whole bunch of code to allow you to decode bitcoind messages/data structures, to participate in the p2p network, and to get access to the rpc/cli interface. It’s definitely not good for use with real secrets/money, though. By comparison, I’ve found other libraries tend to just not have code for the things I’ve been interested in, or they’ve been too out of date to really be useful (eg python-secp256k1 has been unmaintained and out of date, and I see there’s a few “taproot support?” feature requests recently opened on other python modules). I’ve found the test_framework code useful for (a) mining signet blocks (contrib/signet/miner) (b) testing anyprevout/eltoo (c) testing taproot based lightning designs.

But the “don’t want to encourage using this for production work” and “don’t want to make it hard to change things in ways that improve bitcoind” are compelling counter arguments.

I think at a minimum it should be called “bitcoin-test-lib” or something rather than “bitcoincore” to emphasise that it’s not for production use. Maybe it would make sense to heavily version it, so that you say “import bitcoin_test_lib.v22 as btctest” and it’s obvious your code will break when you update to v23. If it were done in a separate repo that just copies code from core once a release, you could support both v21, v22, and v23 simultaneously, which might mitigate backwards incompatibilities.

jamesob commented at 3:08 pm on November 9, 2021: member

@jamesob Have you looked at https://github.com/petertodd/python-bitcoinlib? It also has P2P serialization etc.

I have used this library before, and no doubt it is useful, but I don’t have much confidence in what it provides for P2P/net since no part of its test suite runs against an actual bitcoind instance. Not to mention it doesn’t provide anything along the lines of test_framework.{P2PInterface,NetworkThread,P2PDataStore,...}, which are quite useful and decently complicated (lot of asynchronicity).

MarcoFalke commented at 3:12 pm on November 9, 2021: member

Allowing that the P2P client in the functional tests is overtly coupled to a single Bitcoin Core commit (and not the more general P2P protocol) seems like an unfortunate concession.

Why? If you want to check that older versions of Bitcoin Core can read p2p data from the latest version of Bitcoin Core, you can write a test using previous releases. If that is too much, I think a unit test is better suited to check deserialization compatibility than a functional test. And even if a functional test was the right place, if you wanted to rely on that elsewhere, you’d have to write the serialize logic here first anyway before exposing it.

I think at a minimum it should be called “bitcoin-test-lib” or something rather than “bitcoincore” to emphasise that it’s not for production use.

Agree that this is an acceptable alternative. Though, it would still be good to explain the advantages/disadvantages of the different approaches for non-python packaging people. The sys.path.append hack (https://github.com/bitcoin/bitcoin/pull/23468#issuecomment-963957727) should work for any previous and all future releases already without any patches.

laanwj commented at 11:01 am on November 10, 2021: member

By comparison, I’ve found other libraries tend to just not have code for the things I’ve been interested in, or they’ve been too out of date to really be useful (eg python-secp256k1 has been unmaintained and out of date, and I see there’s a few “taproot support?” feature requests recently opened on other python module

Outside of the specific context of changing the test framework into something it is not, I think this is a good point. I do wish the existing Python libraries for interfacing with bitcoin (e.g. python-bitcoinlib, bit) were more actively maintained. There seems to be a big demand for them (even though @petertodd believes nowadays that Python is a bad choice for that and people should rust instead :smile: ).

DrahtBot locked this on Nov 10, 2022

make Python bindings externally usable #23468

Conflicts