What does @forrestv think?

I don't see any reason to separate submitblock, further cluttering the RPC interface, if the old way will continue to be supported. Other than that, looks good!

I only kept the old way in there for backward compatibility. There's really no reason to use the same method for two different functions - it's like sending coins using getbalance. More importantly, "getmemorypool(<data>)" doesn't provide any way to communicate the reason for rejections. During BIP discussion, some developers expressed interest in keeping the JSON-RPC protocol HTTP-independent, and abusing HTTP headers like getwork does violates this.

There is also a need for passing "features supported" from clients to getmemorypool() so that clients can opt to not implement "noncerange" and inform the server on their support (or lack thereof) of other features (so it might possibly optimize its own processes based on that). So far, nobody has come up with a backward-compatible way to do this; perhaps it would be desirable to ignore non-string , so that a future draft can turn it into an options Object?

Ah, I see. ACK then.

Too late in the 0.6 release cycle for a new RPC call. I think we should pull this early in the 0.7 release cycle so it gets lots of testing.

I disagree with the term bugfix here, but ACK on the changes.

ACK (and agree w/ sipa on term)

Rewrote based on recent BIP 22 revisions, including longpolling support.

Moved longpolling to #1355

ACK, now.

Eligius has been running this from block 179513 (56 blocks found) and EclipseMC from 180573 (11 blocks), totalling 67 valid blocks with no problems found.

Rebased, plus the changes to BIP 22 discussed on IRC (getmemorypool now requires exactly one argument, the parameters Object, but tolerates the old calling methods for compatibility)

Also stripped whitespace when parsing JSON Object in bitcoind-CLI-test-tool (while it could just convert input to Object regardless, it seemed sensible to keep the CLI working with older servers).

I've been trying to send a mail to the mailing list about BIP22, but it doesn't seem to come through.

As it's a bit too long to paste here, you can read it on https://gist.github.com/2909725.

@sipa can you pipe that through "fmt -72" or similar? Even 'raw' requires a horizontal scroll bar, which is unreadable in these modern times...

@jgarzik done

@sipa Hope this addresses everything:

1. Servers are not required to have access to the transaction database, and miners may wish to include transactions that have not been relayed on the main network.
2. The "submit/hash" mutation allows miners to replace actual transaction data with a reference, for servers which support this. The pull-request here does not support this functionality, since it is unlikely to be needed in bitcoind (solo miners and downstream pool servers are probably always going to be local and without bandwidth concerns); there are also mutations which servers can opt to use to allow miners to omit the transaction list entirely when it hasn't been modified, effectively reducing the bandwidth requirements to the same as getwork
3. Your suggestion for a list of required coinbase outputs would involve specifying a format for that list, and actually increases the complexity since the same thing is expressed by what is effectively a boolean right now. I see the benefit of doing it that way, to allow miners to claim fees of transactions they add themselves - but what if a pool doesn't want to allow that?
4. With restricted coinbase input, it is possible for servers to implement BIP 22 in the same way as they currently use getwork, but allowing miners to still audit the block they are working on.
5. Clients are not required to support noncerange limiting, so the added complexity is itself optional
6. "time/*" can usually in practice be ignored, since it is implied if the min/max are provided; it is listed so it is clear the time can be changed, and so servers might have a defined way to express "change the time to anything you want" - it's specified explicitly by bitcoind since there is no practical upper limit to the time miners can send it.
7. The use case for adding transactions is... so miners can add other transactions. Kinda half the point of decentralized mining IMO. :p
8. I don't mind removing "transactions/remove", but I'd rather just note it as being always implied to express that it can be done explicitly
9. "prevblock" mutation is for the scenario where the miner has validated a new block before the server
10. the "required" key on transaction objects is necessary for the flexibility of pools negotiating contracts for transaction acceptance, but "txrequired" allows saving a lot of bandwidth; I agree the mutation should be implicit given these two other methods
11. "mintime" and "maxtime" are needed in addition to their "*off" versions, for servers such as bitcoind which only care about the network rules; in this case, "mintime" is a fixed time, not related to the current time at all.
12. "target" is needed to allow shares of varying sizes; pools do make use of this
13. "workid" is needed to allow the server to identify which work issued the block is being submitted against, so it can properly validate the share
14. Transaction objects must include sigop count, as there is no way for dumb miners to calculate it thanks to BIP16

@luke-jr you didn't respond to the big-picture design question about whether you agree that BIP22 is over-complicated.

I agree with @sipa, I think there are way too many optional features, different ways of doing things, etc.

@gavinandresen "Overcomplicated" is relative based on what it needs to do. I think for the most part (there are exceptions, which I hope to simplify based on sipa's suggestions) BIP22 as it is can't get too much simpler with its given requirements.

I guess @sipa and I think maybe you're throwing in too many requirements. I say start simple, and if there is demand for a feature add it later. I'm OK with planning ahead with a design that allows stuff like adding/removing transactions, but that's a feature I've never heard "dumb miners" say they want.

Also: being explicit about the requirements in the BIP might help. I see only a very vague description of them in the 'motivation' section.

Optional things means fewer requirements, not many. "Dumb miners" don't care about any of this, they're fine using getwork with centralized pools. BIP22 is aimed at "smart miners" which want to (at least) audit the blocks they're working on to keep Bitcoin secure against potential poolop attacks. One practical superiority of decentralized mining is that miners are restored the freedom to choose their own minimum fee policies - that requires being able to pick and choose transactions that go into their blocks. At the same time, pools negotiating bulk fees has been an accepted "plan" for a while, and BIP22 can support that also.

I will try to expand on the Motivation section.

Updated BIP22 based on @sipa and @gavinandresen 's suggestions.

Unless there are problems with the subset of BIP22 supported by bitcoind (in this pull request), however, let's move BIP22 discussion back to the mailing list. I don't think it makes sense to hold up this pullrequest due to unrelated concerns.

Rebased addressing Gavin's most recent comments.

One thing that still bothers me in the implementation is that is supports different decompositions for transactions. I understand the fee/sigops/dependencies/size meta-data is necessary, but do we really need to retain the origin (hex) encoding as well? Sure, dumb miners don't need this, but the protocol is aimed at non-dumb miners.

Smart miners don't necessarily have a transaction database available either. In fact, I'm not aware of a single getmemorypool client that has easy access to one right now.

Also, support for fetching transaction list as hashes exists for non-mining tools (I find it handy via the CLI as a human, to see what transactions are in the mempool). Support for fetching them as hex only is mainly a backward compatibility thing.

@luke-jr What I was talking about in my latest comment here, is the availability of {"tx" : "hex"}, as {"tx" : "obj"} provides a strict superset of that. It would indeed mean breaking backward compatibility, indeed.

So... seems nobody has anything else that needs addressing - time to merge?

Conditional NAK[1]: pick one of DM_OBJ or DM_HEX, not both.

ACK, if one of those is removed.

[1] Red Hat's "conditional NAK" means that if the described condition disappears, then the NAK disappears.

Looks good to a quick review. I'll have to apply the patch and read to be thorough.

Please edit the OP to indicate name change and consensus "why?" opinion.

Will do as soon as we have a final on the new name. I emailed @gavinandresen so hopefully he'll provide input next time he's got email access.

Encore on the name.

Gavin Andresen

On Aug 2, 2012, at 11:02 AM, [email protected] wrote:

Will do as soon as we have a final on the new name. I emailed @gavinandresen so hopefully he'll provide input next time he's got email access.

---

Reply to this email directly or view it on GitHub:
#936 (comment)

The following review comes from reading the code directly, and may or may not reflect a change you made. Regardless, it is something that warranted a note.

1. is mode=foo the preferred form? if yes, update help text to reflect this.

2. help text is missing description of 'time'

3. like 'getwork' this should prevent any progress if !connected || IBD. presently, it only does that check for template mode.

4. if strmode==submit should be "else if"

5. would prefer optional BIP 22 sections moved to another BIP

6. BIP 22 fails to document everything getblocktemplate handles, such as e.g. mutable. Other keys are listed in BIP 22 as required, but your patch does not provide them, like coinbasetxn. Review and sync up code and BIP.

7. Submit mode should mimic getwork, and return true if the block is accepted, or false if not.

1. Neither form is necessarily preferred; what is important is that the code doesn't take a path if it's set to something unexpected.

2. 'time' was for backward compatibility: removed

3. Why should submissions be prevented if not connected? Right now, it's possible this node has no network connections, but is getting its blocks (only) from a JSON-RPC source (perhaps HAM radio?)

4. Changed.

5,6) The pooled mining sections are moved to BIP 23. coinbasetxn is not required, since coinbasevalue is provided, and it's not very trivial to add (it would break the template caching).

7. Accepted design flaws in getwork are not reasonable to propagate into new protocols. (Please review past getwork and BIP22 discussions, and keep in mind bitcoind is not the only or even primary focus of this protocol)

ACK