BIP22: getblocktemplate

luke-jr commented at 2:12 pm on March 13, 2012: member

Replacement for getmemorypool compatible with new BIP 22 specification.

gavinandresen commented at 12:21 pm on March 14, 2012: contributor

What does @forrestv think?

forrestv commented at 3:31 pm on March 14, 2012: contributor

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!

luke-jr commented at 3:52 pm on March 14, 2012: member

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?

forrestv commented at 3:58 pm on March 14, 2012: contributor

Ah, I see. ACK then.

gavinandresen commented at 2:36 pm on March 19, 2012: contributor

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.

sipa commented at 10:52 am on April 4, 2012: member

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

jgarzik commented at 7:36 pm on April 10, 2012: contributor

ACK (and agree w/ sipa on term)

luke-jr commented at 4:46 am on May 13, 2012: member

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

luke-jr commented at 5:15 pm on May 18, 2012: member

Moved longpolling to #1355

forrestv commented at 5:27 pm on May 18, 2012: contributor

ACK, now.

luke-jr commented at 7:19 pm on May 19, 2012: member

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.

luke-jr commented at 5:54 am on June 1, 2012: member

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).

sipa commented at 11:49 am on June 11, 2012: member

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.

jgarzik commented at 12:25 pm on June 11, 2012: contributor

@sipa can you pipe that through “fmt -72” or similar? Even ‘raw’ requires a horizontal scroll bar, which is unreadable in these modern times…

sipa commented at 12:33 pm on June 11, 2012: member

@jgarzik done

luke-jr commented at 1:55 pm on June 11, 2012: member

@sipa Hope this addresses everything:

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.
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
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?
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.
Clients are not required to support noncerange limiting, so the added complexity is itself optional
“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.
The use case for adding transactions is… so miners can add other transactions. Kinda half the point of decentralized mining IMO. :p
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
“prevblock” mutation is for the scenario where the miner has validated a new block before the server
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
“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.
“target” is needed to allow shares of varying sizes; pools do make use of this
“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
Transaction objects must include sigop count, as there is no way for dumb miners to calculate it thanks to BIP16

gavinandresen commented at 2:56 pm on June 11, 2012: contributor

@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.

luke-jr commented at 3:38 pm on June 11, 2012: member

@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.

gavinandresen commented at 4:59 pm on June 11, 2012: contributor

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.

luke-jr commented at 8:06 pm on June 11, 2012: member

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.

luke-jr commented at 9:32 pm on June 11, 2012: member

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.

in src/bitcoinrpc.cpp: in 55fac8aa26 outdated

2235+        aux.push_back(Pair("flags", HexStr(COINBASE_FLAGS.begin(), COINBASE_FLAGS.end())));
2236 
2237-            transactions.push_back(HexStr(ssTx.begin(), ssTx.end()));
2238+        uint256 hashTarget = CBigNum().SetCompact(pblock->nBits).getuint256();
2239+
2240+        static Array*paMutable = NULL;

gavinandresen commented at 1:17 pm on June 12, 2012:

Why is this being cached? If it really needs to be cached, then I’d suggest static Array mutable; if mutable.size == 0 … then initialize…. to avoid memory-leak-checking tools complaining about leaking paMutable at shutdown.

luke-jr commented at 4:03 pm on June 12, 2012:

Why make it slower by not caching it? Adding the static Array idea to rebase.

sipa commented at 11:46 am on June 13, 2012:

I doubt caching it helps a lot, as the values need to be copied into the output Object anyway. Doing that directly is probably just as slow as copying it from a cached value. That said, I doubt it’s the only place where we use allocated objects in static variables, so I don’t really care.

in src/bitcoinrpc.cpp: in 55fac8aa26 outdated

2124             "  \"time\" : timestamp appropriate for next block\n"
2125             "  \"mintime\" : minimum timestamp appropriate for next block\n"
2126             "  \"curtime\" : current timestamp\n"
2127             "  \"bits\" : compressed target of next block\n"
2128-            "If [data] is specified, tries to solve the block and returns true if it was successful.");
2129+            "See https://en.bitcoin.it/wiki/BIP_0022 for full specification.");

gavinandresen commented at 1:22 pm on June 12, 2012:

I think this is insufficient. The help string should describe what subset of BIP 22 is supported by this implementation, or maybe points to a wiki page that describes exactly what this version of bitcoind supports.

luke-jr commented at 4:05 pm on June 12, 2012:

Extended help to be more specific on bitcoind’s implementation.

luke-jr commented at 4:06 pm on June 12, 2012: member

Rebased addressing Gavin’s most recent comments.

sipa commented at 12:22 pm on June 13, 2012: member

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.

luke-jr commented at 2:12 pm on June 13, 2012: member

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.

luke-jr commented at 2:13 pm on June 13, 2012: member

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.

sipa commented at 3:22 pm on June 14, 2012: member

@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.

luke-jr commented at 1:41 am on June 28, 2012: member

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

jgarzik commented at 3:32 pm on August 1, 2012: contributor

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.

jgarzik commented at 4:41 pm on August 2, 2012: contributor

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.

luke-jr commented at 5:02 pm on August 2, 2012: member

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.

gavinandresen commented at 6:02 pm on August 2, 2012: contributor

Encore on the name.

Gavin Andresen

On Aug 2, 2012, at 11:02 AM, Luke-Jrreply@reply.github.com 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)

jgarzik commented at 9:26 pm on August 2, 2012: contributor

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.

is mode=foo the preferred form? if yes, update help text to reflect this.
help text is missing description of ’time'
like ‘getwork’ this should prevent any progress if !connected || IBD. presently, it only does that check for template mode.
if strmode==submit should be “else if”
would prefer optional BIP 22 sections moved to another BIP
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.

jgarzik commented at 9:31 pm on August 2, 2012: contributor

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

Minimal BIP 22 (getblocktemplate) support

- Replaces getmemorypool with new getblocktemplate
- Add missing keys: coinbaseaux, target, mutable, noncerange, sigoplimit, sizelimit, and height
- Accept and send parameter Objects, checking "mode" key if present
- Return rejection reason "rejected" for submit mode

3390014fd0

Merge branch 'gmp_bip_0.6.0' into gmp_bip

Conflicts:
	src/bitcoinrpc.cpp

44427fa833

luke-jr commented at 1:16 am on August 3, 2012: member

Neither form is necessarily preferred; what is important is that the code doesn’t take a path if it’s set to something unexpected.
’time’ was for backward compatibility: removed
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?)
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).

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)

sipa commented at 5:20 pm on August 13, 2012: member

ACK

gmaxwell referenced this in commit 14486dc0e2 on Aug 13, 2012

gmaxwell merged this on Aug 13, 2012

gmaxwell closed this on Aug 13, 2012

suprnurd referenced this in commit 625b5ebb09 on Dec 5, 2017

MarcoFalke referenced this in commit 10b9a811b6 on Jul 22, 2018

lateminer referenced this in commit 12a6b704b6 on Oct 30, 2019

lateminer referenced this in commit 8f2217d2bd on Oct 30, 2019

DrahtBot locked this on May 16, 2023

BIP22: getblocktemplate #936