-
Notifications
You must be signed in to change notification settings - Fork 418
Implement method to submit package of transactions (submitpackage call) #288
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
Conversation
docs/protocol-methods.rst
Outdated
| The Bitcoin Core daemon result according to its RPC API documentation. | ||
|
|
||
| **Result Example** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Not sure if we want to just forward the return value from bitcoind as-is.
The bitcoin core RPC docs say: This RPC is experimental and the interface may be unstable.
If we do want to just forward this result, I guess we too should mark the result as unstable (?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that forwarding the result could cause problems in clients if the e-x server upgrades its daemon to a new version and the result structure changes, while defining our own structure would limit the issues to the e-x server side which seems less problematic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a verbose parameter so the user can decide if he wants the full response or only the package_msg and replaced txids. See PR spesmilo/electrum-protocol#1
157ca93 to
336a16a
Compare
d39138b to
50a161b
Compare
|
Simple script to test the call: import socket
def main():
with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s:
s.connect(("127.0.0.1", 51001))
s.sendall(b'{"jsonrpc": "2.0", "method": "server.version", "params": ["", "1.4.4"], "id": 0}\n')
data = s.recv(1028)
s.sendall(b'{"jsonrpc": "2.0", "method": "blockchain.transaction.broadcast_package", "params": [["raw_parent_tx", "raw_child_tx"], false], "id": 0}\n')
data = s.recv(1028)
print(data)
if __name__ == '__main__':
main() |
|
Commit ff7c1a3 represents the protocol changes in commit spesmilo/electrum-protocol@7bc5f11 |
|
7de390f represents protocol pr status spesmilo/electrum-protocol@3e49ee8 |
SomberNight
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good.
I added some comments but I will fix them myself in a follow-up.
| except ValueError: | ||
| self.logger.info(f"error calculating txids: {traceback.format_exc()}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to import traceback
| except ValueError: | |
| self.logger.info(f"error calculating txids: {traceback.format_exc()}") | |
| except ValueError as e: | |
| self.logger.info(f"error calculating txids", exc_info=e) |
here is some boilerplate:
import traceback
import logging
import sys
logger = logging.getLogger(__name__)
logger.setLevel(logging.DEBUG)
console_stderr_handler = logging.StreamHandler(sys.stderr)
console_stderr_handler.setLevel(logging.DEBUG)
logger.addHandler(console_stderr_handler)and now compare (in a fresh local python interpreter):
try:
txids = [double_sha256(bytes.fromhex(tx)).hex() for tx in tx_package]
except Exception:
logger.info(f"error calculating txids: {traceback.format_exc()}")and
try:
txids = [double_sha256(bytes.fromhex(tx)).hex() for tx in tx_package]
except Exception as e:
logger.info(f"error calculating txids", exc_info=e)and
try:
txids = [double_sha256(bytes.fromhex(tx)).hex() for tx in tx_package]
except Exception:
logger.info(f"error calculating txids", exc_info=True)see https://docs.python.org/3/library/logging.html#logging.Logger.debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that's useful to know!
| if ptuple >= (1, 4, 4): | ||
| handlers['blockchain.transaction.broadcast_package'] = self.package_broadcast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's either not expose the new method at all, or maybe expose it unconditionally by moving it ~6 lines higher into the big dict.
I don't want to gate it behind an arbitrary temporary version number. Once spesmilo/electrum-protocol#2 is done, we can gate it behind that version number along with the other changes.
Until it is documented in the spec, clients should not rely on this method existing at any version.
| raw_txs: a list of raw transactions as hexadecimal strings""" | ||
| self.bump_cost(0.25 + sum(len(tx) / 5000 for tx in tx_package)) | ||
| try: | ||
| txids = [double_sha256(bytes.fromhex(tx)).hex() for tx in tx_package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these txids for? They are in reversed byte order versus normal RPC interface, so logging them isn't that helpful, because it's not like you can copy-paste them to an explorer to view the txns (unless you manually reverse them first).
Also, they are wtxids, and not normal txids, because you are hashing the raw txn which contains witness data, rather than the stripped txn...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good catch. Yeah this is incorrect i remove it in this followup PR #319
Removes the incorrect and unnecessary txid calculation in `ElectrumX.package_broadcast()`, the daemon will complain anyways if the transactions are invalid.
session: broadcast_package: followup #288
This implements the remainder of the protocol 1.6 changes, and it bumps PROTOCOL_MAX to "1.6". Some of protocol 1.6 was merged previously, see e.g.: - #288 - #302 - kyuupichan/electrumx#1001 - #325 ref spesmilo/electrum-protocol#6 ref #317
Defines a protocol method
blockchain.transaction.broadcast_packagefor protocol version 1.4.4 and implements it so clients can submit a package of transactions to the mempool using the bitcoin coresubmitpackagemethod. This allows clients to broadcast a package of transactions that would on their own not be accepted to the mempool (e.g. below purge fee parent and high fee child to bump)