protocol 1.6: minimal update to get broadcast_package out #6
Conversation
This is a minimal changeset for a new protocol version. The ideas for the less trivial changes are deferred a bit, just so we get something out. ref spesmilo#2 ref spesmilo/electrumx#90
|
Concept ACK 7b84d2a, thanks! |
|
ACK, this is a useful update, and I think it makes sense to make incremental changes. I'll save my requests for the next version :) |
Looks very reasonable! regarding the concatenation -> list for headers — what is the rationale for this? my biggest concern here is I really hate apis that return different results based on versioning — although I know that is the philosophy behind the electrum protocol — it just makes me uneasy that some wallet dev or some api out there will have extra burden created by a breaking api I would prefer adding a new method name that returns list or having it take an optional extra arg to indicate list Rationale again is I fear negative value is created for the world every time some devs (in this case us) introduce api breakage and in this case it’s not like there’s that much burden on us to continue to send it as a concatenation… literally in python joining a string from a list is implemented on the C side anyway so.. Yes in hindsight it would have been more aesthetic had the headers response been a list… for sure. but again I feel more burden is exacted on the world now to change it.. my two cents. edit: also note it’s faster in general in most json libs to parse 1 giant string that is N 80-byte headers concatenated than it is to parse a list of N items, each being a string of 80 bytes .. so there’s that.. but that is minor |
| * *headers* | ||
|
|
||
| The binary block headers concatenated together in-order as a | ||
| hexadecimal string. Starting with version 1.4.1, AuxPoW data (if present | ||
| in the original header) is truncated if *cp_height* is nonzero. | ||
| An array containing the binary block headers in-order; each header is a | ||
| hexadecimal string. AuxPoW data (if present in the original header) is | ||
| truncated if *cp_height* is nonzero. |
There was a problem hiding this comment.
Looks very reasonable!
regarding the concatenation -> list for headers — what is the rationale for this?
my biggest concern here is I really hate apis that return different results based on versioning — although I know that is the philosophy behind the electrum protocol — it just makes me uneasy that some wallet dev or some api out there will have extra burden created by a breaking api
I would prefer adding a new method name that returns list or having it take an optional extra arg to indicate list
Rationale again is I fear negative value is created for the world every time some devs (in this case us) introduce api breakage and in this case it’s not like there’s that much burden on us to continue to send it as a concatenation… literally in python joining a string from a list is implemented on the C side anyway so..
Yes in hindsight it would have been more aesthetic had the headers response been a list… for sure.
but again I feel more burden is exacted on the world now to change it.. my two cents.
edit: also note it’s faster in general in most json libs to parse 1 giant string that is N 80-byte headers concatenated than it is to parse a list of N items, each being a string of 80 bytes .. so there’s that.. but that is minor
regarding the concatenation -> list for headers — what is the rationale for this?
That change was originally asked for Namecoin as it has variable sized headers.
For any of our use cases, there is no practical gain at all. In fact it makes the response size around 2% larger :P
Nevertheless, it is a conceptual clean-up, and even if we don't care about altcoins I think it's nice to do this small cheap generalisation here.
The earliest non-IRC discussion about it is probably:
spesmilo/electrumx#109
I also think putting this small breaking change into the API serves as a sanity-check whether an implementation that bumps the version number really read and implemented the spec :)
One of the main points of having a version number and negotiation is to allow making breaking changes.
I would prefer [...] having it take an optional extra arg to indicate list
I don't want the API to offer a choice between concatted str and a list. Would rather keep the API simple.
I would prefer adding a new method name that returns list
Sure, if you think that's better, we could rename the method to make the breaking change obvious. (i.e. rm the old method and add a new method with a different name)
But then I would remove the current method (so just do a rename). I already gave this some thought in the context of blockchain.relayfee vs mempool.get_info, where we could e.g. mark blockchain.relayfee as deprecated and then only rm it in a later version, but as there is a direct replacement API, I think there is no point and we should just rm it right away. Same thinking here if we rename it.
Ultimately it's the least interesting change, so if controversial, we could also drop it.
There was a problem hiding this comment.
variable sized headers.
Ahh. Well that settles it, then. Yes, if the protocol is to support such coins, one needs this.
bumps the version number really read and implemented the spec :)
Ha ha.
Sure, if you think that's better, we could rename the method to make the breaking change obvious. (i.e. rm the old method and add a new method with a different name)
No, it's fine. Since you prefer simplicity (as do I), it's ok. Yes, that is a good point about namecoin needing this and a good point as well about the entire point of version negotiation. I think it's fine as proposed, then.
As for mempool.relayfee vs mempool.get_info.. I also think as proposed it's fine. No need to change anything. Stick to the proposal you have so you can get this out the door faster.
There was a problem hiding this comment.
(it's nicer to keep discussions threaded)
That change was originally asked for Namecoin as it has variable sized headers.
Making a change to
blockchain.block.headersis unnecessary for this (niche) use case - Namecoin can already use JSON-RPC batching withblockchain.block.headerto retrieve a bunch of variable sized headers. Given this, consider whether making this breaking change actually does more harm than good?
Using JSON-RPC batching to fetch n single headers compared to a single RPC to fetch a list/concat increases the upstream bandwidth to around n times as large (and the downstream to around 20% more - less if the headers are much larger than 2*80 bytes). (Then as n reaches the chunk size, e.g. 2016, the factor stops growing. Requesting 10**6 headers only increases the upstream bw to 2016x)
It is also higher CPU load on the server to process 2016 RPCs vs just 1.
So yes, indeed requesting headers one-by-one is a workaround. Though there is a reason this separate RPC for requesting a chunk of headers exists in the first place - it is purely for efficiency.
EDIT: I can demonstrate why it creates more work for wallet devs even if they implement version negotiation correctly.
Any change in the protocol creates some work for client/server devs, even more so if they want to support multiple protocol versions at the same time.
There was a problem hiding this comment.
Any change in the protocol creates some work for client/server devs, even more so if they want to support multiple protocol versions at the same time.
Fair enough. However I'd like to say that when one "breaks" an existing API, in order to avoid annoying people, it's nice to also "reward" the dev who now has extra work to do.. with some new feature or facility on account of the fact that he was asked to do some extra work. This is how one makes people happy, and keeps them from being resentful.
In this case: an API nobody ever worries about (headers), now needs to be worried-about, with 0 or almost no benefit for 99.9% of code out there. Would have been less antagonistic to users of this protocol to just offer up a blockchain.block.headers2 or somesuch for the headers-as-list people. Just sayin'.
Anyway yeah I'm fine with moving ahead and not belaboring this point any further... let's get 1.6 out the door.
|
looks good, a4f158c |
Making a change to |
I'm glad you're also conservative on this as am I. I am willing to go either way on it but I do think FWIW that it will be annoying to devs to have to worry about this incongruity in the API; if it were 100% up to me I would have just created a second method with a different name that does almost the same thing, but returns results in an alternate format (headers-as-list). Why? Because on top of the incongruity argument, even on v 1.6 it would save an inconsequential number of bytes (~4%) on the server and client-side when serializing/sending/receiving/parsing the JSON. So there is a slight slight slight perhaps immaterial but nonzero advantage to status quo for non-variable-sized-header chains. FWIW, though, I'm also OK with the explanation given and given that this protocol does support version negotiation, that's what that facility is for. And, on some level, returning a big blob of headers feels "ugly" and returning them as a list is more "aesthetically pleasing". So yeah, I'm ok with either way but I do appreciate the sentiment we share that breaking changes are sorta uncomfortable... EDIT: I can demonstrate why it creates more work for wallet devs even if they implement version negotiation correctly. Say you are a wallet that would like to make use of the new 1.6 API, (perhaps My two cents. The reason why I argue for this so fervently is i am exhausted with e.g. Apple for example breaking old APIs and creating busy work for devs. Sometimes it feels like I'm swimming in slippery pasta when I program natively for iOS and/or macOS and I dare to bump up the SDK version... So I'm like.. kinda traumatized by APIs that do that -- create extra new work for you just to get back to what was working before. That being said, I'm ok with just pushing forward as is proposed. Would be nice to just have the new version out, regardless -- just in future breaking old ancient stuff that was working just fine is kind of just creating busy work for devs so.. /end rant |
|
|
||
| .. note:: The exact relay behaviour might depend on the bitcoind version of the server. | ||
|
|
||
| .. note:: Server implementations should verify (e.g. by enforcing a minimum bitcoind version at runtime) |
There was a problem hiding this comment.
Umm.. should we be adding this to the server.features map as a keyword? Maybe submitpackage? This way wallet clients know right away if the RPC exists? Or are we just going to specify they can expect errors from this RPC if the underlying bitcoind submitpackage RPC is missing and be ok with that? Or are we going to specify that all "1.6 compliant" implementations for BTC must be running on Core >= 28.0.0?
There was a problem hiding this comment.
In theory the backing bitcoind could also be btcd, etc. It would be nice not to "require" Bitcoin Core in the protocol itself.
Specific server implementations can require Bitcoin Core or specific versions of Core, that's fine I think, but the protocol ideally should not. Maybe btcd has an equivalent RPC to submitpackage and a server implementation is then free to use that.
The expectation is that if the server claims to support protocol version 1.6, they should have a working blockchain.transaction.broadcast_package. Hence I see no point in putting stuff into server.features for this.
There was a problem hiding this comment.
The expectation is that if the server claims to support protocol version 1.6, they should have a working blockchain.transaction.broadcast_package. Hence I see no point in putting stuff into server.features for this.
EDIT: Oh, I see, @SomberNight said he is opposed to claiming 1.6 if this facility is missing.
Well, fine. I'm adding the key anyway. I will support "1.6" on Fulcrum with or without
submitpackagebeing present. Fulcrum runs multi-coin anyway so requiring something that doesn't even exist on other coins as a facility for a new protocol version seems crazy to me.. but you guys do what you like. I'll continue to maintain my "almost 100% compatible" specification, which will be more loose/liberal than yours, I guess, and will denote features supported via features flags...
electrumx requires Bitcoin Core 28.0+ if COIN=Bitcoin is set. Couldn't you just special-case BTC similarly: require a working submitpackage only in that case? Obviously for the other coins do whatever you feel like.
There was a problem hiding this comment.
How does everybody here feel about adding a key to the
server.featuresmap to indicate thatblockchain.transaction.broadcast_packageis supported?
If you have logic to determine whether broadcast_package is supported, even for non-Core bitcoinds (e.g. btcd), I think it would be better to hard crash and tell the server operator to upgrade their bitcoind.
What exactly is the scenario you are trying to support?
- is it for "BTC", using old "Bitcoin Core"? in that case the server should either not advertise 1.6 -- or just crash and tell the operator to upgrade bitcoind
- is it for "BTC", using "btcd"/etc? in that case you/we should research if btcd has an equivalent RPC, figure out how to test for the presence of that, and behave similarly as in the "Bitcoin Core" case
- is it for other coins with daemons that do not have submitpackage or care about the package-relay concept at all? sure the RPC does not make sense there and feel free to leave it non-functional or whatever
Is it that in the case of (1), you don't want to force the operator to upgrade their bitcoind? Why? If they upgraded the electrum server, why can they not be expected to also upgrade bitcoind around the same time?
There was a problem hiding this comment.
you don't want to force the operator to upgrade their bitcoind? Why?
Does this even need a reply? If the answer to this question is not self evident to you — nothing I can say here will help.
but let me try:
it appears you are some sort of centralist or statist in philosophy, if I may be frank. That’s fine about 60% of the population has this mentality so it’s normal.
but that’s besides the point:
the role of this software is not to dictate terms to people but rather to help people do what they want to do, within reason.
also the entire point of the soft fork philosophy bitcoin core is in is precisely because they don’t want to “force” upgrades on nodes.
given that Bitcoin BTC is this way — it is doubly silly to force people to upgrade to latest core, when BTC already is paying a considerable cost in terms of technical debt to always soft fork and never hard fork.
Given that BTC has already paid this cost in order to benefit people from not being required to upgrade — it’s silly to then turn around and have the philosophy to force upgrades.
sorry, I’m not gunna play this game of dictating policy on people. I’m a libertarian and a voluntaryist. I try to offer people choice and not shove things down their throats by leveraging influence due to popularity of my software …
but you do you — like I said this is open source we are free to voluntarily participate or not.
There was a problem hiding this comment.
just crash
Are you serious?
Yes. I meant gracefully exit with a human-readable error message, rather than e.g. a segfault. I hope that was obvious.
you don't want to force the operator to upgrade their bitcoind? Why?
Does this even need a reply? If the answer to this question is not self evident to you — nothing I can say here will help.
Do you also support bitcoin core 0.3 then? Where do you draw the line? It is actively harmful to run a public server that auto-connect clients might connect to if it's backed by an ancient bitcoind. Old bitcoinds often have very strict relay policies. For example, the user would not be able to broadcast a tx paying to a taproot address. That is actively harmful. Do you want to expose that in server.features too? Whether the client is allowed to pay to a taproot address?
(EDIT: to be clear, paying to a taproot address is just one example, I can come up with several more if needed. For example, paying to a segwit v0 address was added in 0.13, then paying from was added in 0.13.1, then there was the CPFP carveout which is needed for lightning, then there was spending from a taproot address (added later than paying to), then there are recent datacarrier size changes, there is pay-to-anchor, v3 transactions, etc. Do you want to make all this optional and signal it one-by-one?)
EDIT2: Most of the standardness/relay changes are either already needed by Lightning or will be relied on in the future. A client wanting to use LN expects them to be supported by servers.
There was a problem hiding this comment.
Your philosophy is refuse to claim support for 1.6 if submitpackages is missing — with no consideration for other coins or networks where the entire facility is absent as a concept. You also recommend hard exit on lack of this RPC rather than warning and features map stuff. You also seem to be deeply shocked that I wouldn't be on-board with "forcing" people to do things.
I suspect this is from a lack of ability to deal with heterogeneity. A need for homogeneity -- to wrangle chaos into simplicity... at the expense of consideration for others?
Or.. Not sure if you’re being careless, or are on a power trip… or what.
The language you use “crash the node” and “force upgrade” is reckless and betrays a lack of respect for other people.
Maybe English isn’t your first language and maybe in German such concepts are not offensive but to me your linguistic choices in how you express yourself are distasteful. They reveal a sense of importance that is bordering on abusive behavior… which is a trap software authors fall into.
I am trying to warn you that this is bad design. It's far better to make use of the features map esp. since the rate of adoption of >= v28 Core/Knots is low. You don't want to antagonize your admins. It's better to warn on startup like I do here: cculianu/Fulcrum@06b50dc#diff-3884123fb22804742dac29791cafa44d2305af37f752aada08460afff7d5c4dcR319-R320
Let me say this: This is 1 relatively new method that may not even exist on other coins.
Nodes capable of even offering this method make up maybe 20% of the BTC network at the present time.
Yet your solution is to insist that you cannot be 1.6 or above as an electrum server if this method is missing
This is a method that is only on a few coins (BTC, Namecoin, and some others)
Last I checked this is a multi coin protocol.
You are literally requiring a feature that only exists on 2-3 coins out of N as a requirement for this protocol going forward.. from now until the end of time. Nodes or even entire coins lacking submitpackages are forever relegated to 1.4 or 1.5 protocol version for all eternity — is what you are proposing.
Can you not take a look at yourself honestly here ?
And all for submitpackages which is experimental as per the rpc docs
Comparing it to segwit or taproot or v0.0.3 other stuff is not honest argumentation on your part (you are intentionally not arguing in good faith here. Stop being manipulative and be reasonable!). Also during a transition period where new features are introduced -- is why you have a features map in the first place! Also -- wallets that care about submitpackage need to check the features map anyway for protocol version.. so it's not like even an extra thing is needed to determine yes/no on that feature. But more to the point: You don't "force" people to do stuff. You nag them and you offer gentle reminders. And you offer a transition period upgrade path. "Forcing" people to do things is a nice way to "lose" people.
You don’t see a problem with that?
Also about core v28.0.0 in particular -- it bumped up the glibc version so if you are on like Ubuntu 20 or below I don't think it will even run unless you recompile it (but then you need to figure out how to get a new enough C++ compiler for it on an older system). It's not like forcing people to go to v28.0.0 doesn't have any conceivable headaches associated with it.. it may, for some admins, in some circumstances.
Also you are refusing to allow the specification to make room for cases where this feature is absent (such as in other coins) — it’s not even considered as a possibility in your world-view or in your specification!
I’m sorry I refuse to participate .. you are either intentionally obtuse or on some power trip.
I’ll just adopt 1.6 in a more flexible fashion than the draconian thing you have proposed.
open source after all…
thanks for the chat
EDIT: If you account for Knots (which also has this method) it's more like 39%-49% or thereabouts (not sure how those Knots numbers map to version numbers tbqh). Still not a majority feature. Just sayin'. "Forcing" is .. not cool. server.features exists... use it!
There was a problem hiding this comment.
To get facts out first, around 70% of the Bitcoin network runs Core 28+ or Knots 28+ . (see https://luke.dashjr.org/programs/bitcoin/files/charts/branches.html).
Also note that electrum server operators typically upgrade faster than the average node operator. Also note that I am suggesting tying the upgrading of the electrum server itself to the upgrading of the bitcoind, in this case. Many people running old bitcoinds are unlikely to run a new electrum server.
But this is not even the main point. The main point is that I was only suggesting this for BTC, not for altcoins.
My quotes:
Couldn't you just special-case BTC similarly: require a working submitpackage only in that case?
for BTC we can require it
I was suggesting we make submitpackage mandatory for electrum protocol 1.6 for bitcoin only. For BTC only. Not for the other altcoins. For those, like I said already, you could declare 1.6 without the new RPC.
Again, for every other altcoin, the server can declare 1.6 regardless of the RPC.
The server knows what coin it is being run for. We have already made coin-specific distinctions in the protocol spec in the past. It is not without precedent, if that matters. You very clearly know this as you have made bcash-specific changes yourself. There are methods that are only available for Namecoin or Dash or other random altcoins.
Your philosophy is refuse to claim support for 1.6 if submitpackages is missing — with no consideration for other coins or networks where the entire facility is absent as a concept
What do you mean "no consideration"? I very clearly was considering other coins in my comments. I was asking you to explain your thinking. I even asked this, to which if you replied positively, I would have considered this whole thread resolved:
Ah. Are you thinking about the scenario, where an altcoin that does not care about broadcast_package now, might introduce it in the future, after 1.6 is widely rolled out? For that case, feature flags might indeed be useful.
but no, instead you called me a statist and started ranting about philosophy.
Yes sure, I am against freedom and that's why I have spent a considerable amount of my life working on Bitcoin. Good catch. Believe whatever you will.
Nodes or even entire coins lacking submitpackages are forever relegated to 1.4 or 1.5 protocol version for all eternity — is what you are proposing.
Please quote the exact sentence where I implied anything of the sort.
I am trying to warn you that this is bad design. It's far better to make use of the features map esp. since the rate of adoption of >= v28 Core/Knots is low. You don't want to antagonize your admins. It's better to warn on startup like I do here: cculianu/Fulcrum@06b50dc#diff-3884123fb22804742dac29791cafa44d2305af37f752aada08460afff7d5c4dcR319-R320
No, it's not better to just warn the operator. It is an alternative reasonable approach, but it is not objectively better. I explained how it is actively harmful to run a public electrum server participating in peer-discovery that is backed by an old bitcoind. Please try to refute my line of thinking there if you disagree on this specific point.
But in short, clients would unknowingly connect to such servers and even in the best case get back error messages when broadcasting a tx: users will then complain on the forums / irc / github, that stuff "does not work" and it will take 2-3 rounds of back-and-forth to figure out what is going on. Don't know about you but I don't fancy doing level1 support much.
And that's assuming the best case, when there is a clear error message that gets triggered on a user-initiated action.
Now imagine automatic logic needing to force-close a lightning channel and getting errors on tx-broadcast - that's not user-initiated action. Not a scenario you want to get hard-to-decipher errors coming from bitcoind and passed on to the server. Or not even getting an error, because the client connected to bitcoin core 26 and submitpackage "works" but does nothing. Sure, the client can have logic to try to deal with all this and ultimately will have to handle even silent failures, but please, let's try to keep the practically occurring common cases working by default with a high probability.
Comparing it to segwit or taproot or v0.0.3 other stuff is not honest argumentation on your part (you are intentionally not arguing in good faith here. Stop being manipulative and be reasonable!).
Was calling me a statist and saying I am on a power trip done in good faith? :)
I listed many policy changes that happened over the years and are scattered throughout time. Some of them are actually very recent, from 28.x and even 29.0 (!!) (e.g. ephemeral dust). I highlighted taproot to give a well known example. Sure, taproot support is widespread now, so you could argue all nodes support it. For context, taproot spending support was added in 0.21, released 2021-01.
Then again, in spesmilo/electrumx#318, you were asking about e-x's reliance on estimatesmartfee and its mode arg, which was added in 0.15, released 2017-09. That's why I cheekily asked if you also want to support bitcoin core 0.3 -- do you really not want to assume a younger than 8 year old bitcoind? So I guess taproot is old enough to be an unreasonable example but estimatesmartfee, well, who knows if bitcoind supports that?
Out of curiosity, what is the minimum version of Bitcoin Core that Fulcrum supports?
Also, do you regularly test with that version when you make changes to daemon RPC calls?
Also note that adding a broadcast_package key to server.features as you suggested has very limited usefulness if only Fulcrum does it. In that case it is only really useful if Fulcrum tries hard to put broadcast_package=false in the map. The "missing" and "true" keys cannot be reliably filtered for if others don't put true values in their map. Maybe you already realised this, I just wanted to highlight this point.
Let me explicitly state what I would have thought was already apparent from my previous comments.
I think it would be reasonable and I am asking if it's acceptable if we
- put a tri-state (false/missing/true) marker into
server.featuresfor submitpackage, regardless of coin (so both for BTC and for others) - for BTC only, we require
submitpackage_marker=truefor protocol version 1.6. That is, we guarantee (on a best-effort basis I guess) that the new RPC is available and works when the new protocol is negotiated. (for BTC only)
Alternatively, we could also make the new submitpackage-based RPC specific to BTC. Just like there are Namecoin/Dash-only RPCs, it would only be exposed if COIN=BTC. No other coin cares about it anyway.
There was a problem hiding this comment.
In theory the backing bitcoind could also be btcd, etc. It would be nice not to "require" Bitcoin Core in the protocol itself. Specific server implementations can require Bitcoin Core or specific versions of Core, that's fine I think, but the protocol ideally should not. Maybe btcd has an equivalent RPC to
submitpackageand a server implementation is then free to use that.The expectation is that if the server claims to support protocol version 1.6, they should have a working
blockchain.transaction.broadcast_package. Hence I see no point in putting stuff intoserver.featuresfor this.
Agree with @SomberNight here. I don't think it makes sense to make this an 'optional' feature.
|
|
||
| .. note:: Server implementations should verify (e.g. by enforcing a minimum bitcoind version at runtime) | ||
| that the backing bitcoind supports relay of transaction packages. For example, note that Bitcoin Core 26.0 | ||
| already exposes the `submitpackage` RPC however it is effectively non-functional until Bitcoin Core 28.0. |
There was a problem hiding this comment.
How is it effectively non-functional until 28.0? I never used this RPC -- is it totally broken in 27? Or is this maybe a typo?
According to these docs it looks almost identical to 28.0.0, as early as 27.0.0: https://bitcoincore.org/en/doc/27.0.0/rpc/rawtransactions/submitpackage/
It only lacks those two optional args: maxfeerate and maxburnamount...
There was a problem hiding this comment.
The submitpackage rpc is not broken pre 28 but the functionality to relay (some) transaction packages was only merged in v28 with bitcoin/bitcoin#28970.
So if the Electrum server was running on a node without any form of package relay (e.g. core pre v28) a wallet sending a transaction package to the server wouldn't get an error message but the transactions wouldn't get relayed either, being held only in the mempool of the server.
There was a problem hiding this comment.
That's crazy to me. And useless. Ha ha.
Ok, thanks for the info. I'll update Fulcrum accordingly and require Bitcoin Core v28.0.0 to offer up this RPC.
|
|
||
| 5. Order mempool transactions by ``(-height, tx_hash)``, that is, | ||
| ``0`` height txs come before ``-1`` height txs, and secondarily the | ||
| txid (in network byteorder) is used to arrive at a canonical ordering. |
There was a problem hiding this comment.
Note that the byteorder used for the sorting is not the human-readable txid byteorder but the network one.
I chose this because it fits the existing internals of electrumx much nicer.
However in cculianu/electrum-cash-protocol#5 (comment) @cculianu expressed frustration about this choice.
I guess I don't mind either way though.
Any strong opinions?
There was a problem hiding this comment.
I appreciate your asking others for feedback on this.
the network one.
Just to be annoying -- I'll smartly say that it can be argued the "network" order is only useful if you speak the p2p protocol. Otherwise it can be argued the true network order is the RPC order. :P
There was a problem hiding this comment.
Any strong opinions?
Network byteorder makes sense to me for such an internal behavior, IMO. But, the most important thing is that it's well defined.
|
How does everybody here feel about adding a key to the Rational: I am definitely adding this key to Fulcrum. Since unlike ElectrumX, I do plan on continuing to support older or different bitcoind's, etc. So, it's entirely possible some servers out there will not have this RPC, so it's only fitting to advertise whether they have it or not in Here is the proposed text for this as I'm adding it to my documentation now (under the docs for
EDIT: Oh, I see, @SomberNight said he is opposed to claiming 1.6 if this facility is missing. Well, fine. I'm adding the key anyway. I will support "1.6" on Fulcrum with or without |
Not a fan of a feature flag just to add support for a new RPC call.
Please don't advertise support for v1.6 then. In my experience with different implementation |
|
|
||
| 5. Order mempool transactions by ``(-height, tx_hash)``, that is, | ||
| ``0`` height txs come before ``-1`` height txs, and secondarily the | ||
| txid (in network byteorder) is used to arrive at a canonical ordering. |
There was a problem hiding this comment.
Any strong opinions?
Network byteorder makes sense to me for such an internal behavior, IMO. But, the most important thing is that it's well defined.
|
|
||
| If *verbose* is :const:`true`: | ||
|
|
||
| The bitcoind response according to its RPC API documentation. |
There was a problem hiding this comment.
Can we add a link here, esp. given that Bitcoin Core's RPC docs are not always super easily discoverable?
|
|
||
| .. note:: The exact relay behaviour might depend on the bitcoind version of the server. | ||
|
|
||
| .. note:: Server implementations should verify (e.g. by enforcing a minimum bitcoind version at runtime) |
There was a problem hiding this comment.
In theory the backing bitcoind could also be btcd, etc. It would be nice not to "require" Bitcoin Core in the protocol itself. Specific server implementations can require Bitcoin Core or specific versions of Core, that's fine I think, but the protocol ideally should not. Maybe btcd has an equivalent RPC to
submitpackageand a server implementation is then free to use that.The expectation is that if the server claims to support protocol version 1.6, they should have a working
blockchain.transaction.broadcast_package. Hence I see no point in putting stuff intoserver.featuresfor this.
Agree with @SomberNight here. I don't think it makes sense to make this an 'optional' feature.
| ] | ||
| } | ||
|
|
||
| When *verbose* is :const:`true`:: |
There was a problem hiding this comment.
If we really care that this endpoint could ever be implemented by something else than Bitcoin Core, it might make sense to simplify the return values (esp. the verbose one), as they are very specific to Bitcoin Core. However, the chances are rather slim that we'll see any other implementation supporting package relay/TRUC any time soon.
6 participants
This defines a new protocol version: 1.6, even more minimal than what was previously in #2, just to get something out quickly.
Compared to the existing protocol version 1.4.3:
blockchain.transaction.broadcast_packageto expose bitcoind'ssubmitpackagemempool.get_info, to expose a subset of bitcoin core'sgetmempoolinfoblockchain.relayfeeredundant, which is now removedthe
server.versionmessage must be the first message sent.That is, version negotiation must happen before any other messages.
backwards-compatible way: mempool txs now have a canonical ordering
defined for the calculation (previously their order was undefined).
blockchain.scripthash.get_mempoolpreviously did not definean order for mempool transactions. We now mandate a specific ordering.
modeargument added toblockchain.estimatefeeblockchain.block.headersnow returns headers as a list,instead of a single concatenated hex string.
The changes here are trivial to implement and hence I believe we can release this quickly.
The rest of #2 (such as outpoint subscriptions and witness-spv) is postponed for protocol version 1.7, which I also intend to have "soon", but the changes in the current PR (notably submitpackage) is even more urgent.
After that, there are still plans even for 2.0, e.g. the history pagination... :)