namecoin / electrum-nmc

Namecoin port of Electrum Bitcoin client.
https://www.namecoin.org/
MIT License
29 stars 24 forks source link

Replace concatenated headers with JSON array #216

Open JeremyRand opened 4 years ago

JeremyRand commented 4 years ago

The Electrum protocol sends headers from the server to the client as a string of concatenated headers. This is problematic for AuxPoW coins and any other coins that use variable-length headers; Electrum-NMC is forced to maintain a fairly heavy set of patches against upstream to work around this.

It would be a lot better to change the protocol to use a JSON array of single headers. This would simplify Electrum-NMC's codebase substantially.

Neil from ElectrumX has stated that he's fine with merging a PR for this (the change would be contingent on a protocol version bump to 1.5).

JeremyRand commented 4 years ago

@domob1812 is it okay to allocate up to 3 days of Handshake funding to this, if a developer is interested?

domob1812 commented 4 years ago

It certainly sounds like a useful project. I'm not sure if we should fully fund cleanup-only projects like this, though. IMHO it is better to either just give a smallish bounty in case someone still wants to do it (but not pay full time for it), or to allocate funds to a concrete project with benefits beyond cleanup where this could be a part of (i.e. necessary refactoring for implementing whatever the project is otherwise doing).

This is not a very strong opinion, though, so if others disagree, I have no objections to fully funding this.

JeremyRand commented 4 years ago

@domob1812 From my point of view, a quite large fraction of my time spent on Electrum-NMC is occupied by dealing with merge conflicts due to Electrum-NMC's way-too-large patch size relative to upstream. Refactors like this, which allow Electrum-NMC to stay much closer to upstream Electrum, save me a huge amount of time in the long run, and allow me to focus on new features instead. So I think it's absolutely worth spending some funds on.

More generally, we've had a problem for the past few years where refactors/cleanup and other "behind the scenes" tasks weren't getting funded, because almost all of the funding was earmarked for new shiny features. This has resulted in a lot of code cleanups never happening (or only happening without pay), and there's a pretty large amount of ugly code that's built up as a result. Now that we can finally allocate funds however we like (since the Handshake funding isn't earmarked), I do think a significant portion should go to cleanups/refactoring/code-quality tasks, at least in cases where it's clear that the benefit would be significant.

On a more practical note: Neil is happy to take a PR but he's going to do a release within the next few days, so he needs the PR written and submitted ASAP if it's going to get into the next release. Neil isn't planning to do any additional releases for a while after this one. So this particular task is somewhat time-sensitive and may need to be fast-tracked in terms of funding approval. I've already found a dev who's up for implementing it.

domob1812 commented 4 years ago

Like I said, my opinion on this is not very strong. If you think this is worth it, and perhaps someone else from the community agrees (but even if noone feels like commenting), I'm fine with it.

But does that mean I should submit requests for funding as well for future cleanups, e.g. for spending a day recently on cleaning up the "mess" created by Bitcoin PR #17809 in Namecoin RPCs?

JeremyRand commented 4 years ago

@domob1812 I assumed that your Namecoin Core work (including keeping the diff against upstream reasonably minimized) was already funded via Xaya; if some of it isn't, then I'd probably have no problem with you requesting Namecoin funding for it.

domob1812 commented 4 years ago

Yes it is, but then I guess Xaya / AW Ltd could request funding for the work time I spend on it every week. (Obviously from now on, since the past work is a separate thing.)

(I don't think we plan to do that.)

JeremyRand commented 3 years ago

@ahmedbodi I've tested your ElectrumX branch. It mostly works, but there's a small bug that breaks things. You're returning a list of bytes for the headers field in the result of block_headers_array; this should be a list of hex strings instead. I.e. instead of appending header to result['headers'], you should be appending header.hex().

Also, I see a bare return in the AuxPoW version of block_headers; I'm pretty sure that should be return result.

ahmedbodi commented 3 years ago

gotcha, will take a look at this as soon as im well again