[RPC] Add optional locktime to createrawtransaction

dgenr8 commented at 6:30 pm on March 22, 2015: contributor

A non-zero locktime also causes input sequences to be set to non-max, activating the locktime.

gmaxwell commented at 6:36 pm on March 22, 2015: contributor

So… an extra parameter is kind of a pain because if we add any additional ones after it it becomes a pain to have to dummy out this one; so we should take care to make sure that this is really the order that we want the arguments in, and that maybe we don’t want something else first. One way around this would be to take an array for additional named arguments.

Is there a reason that the locktime=N parameter of bitcoin-tx doesn’t already accommodate this functionality? You can createrawtransaction then use bitcointx to set the locktime. Though right now it doesn’t set the sequence numbers but perhaps it should.

dgenr8 commented at 7:26 pm on March 22, 2015: contributor

Locktime is the only thing left at the same level as vin and vout, so it seems logical that it should be adjacent to them. Wouldn’t more complex scripts, for example, be accommodated by adding flexibility in params[1]?

I would like to use this in a python test, where adding in bitcoin-tx would be kind of messy.

gmaxwell commented at 7:31 pm on March 22, 2015: contributor

Transaction version is as well (and consider, upcoming anti-malleability BIP that makes the choice of transaction version an important decision). There may be other-meta-parameters like controls over ordering (e.g. the output address array is unordered.). I’m not sure how much this matters.

sipa commented at 11:40 am on March 23, 2015: member

Perhaps we should have support in the python test framework to use bitcoin-tx instead of createrawtransaction.

jgarzik commented at 11:45 am on March 23, 2015: contributor

“weak NAK” (that is, NAK, unless somebody can talk me out of it)

bitcoin-tx can already do this.
The “pure function” RPCs are on the road to removal long term. We should not be extending pure-function RPCs.

dgenr8 commented at 12:54 pm on March 23, 2015: contributor

@jgarzik I just needed this, and thought it was harmless. No problem if I was mistaken.

dgenr8 commented at 1:31 pm on March 23, 2015: contributor

Ping #5503, #5524.

dgenr8 renamed this:
~~Add optional locktime to createrawtransaction~~
[RPC] Add optional locktime to createrawtransaction
on Mar 23, 2015

petertodd commented at 9:18 am on March 26, 2015: contributor

@sipa Or just depend on a more featureful Bitcoin library - would be pretty trivial to do in python-bitcoinlib…

jgarzik added the label RPC on Apr 5, 2015

jonasschnelli commented at 6:11 pm on April 7, 2015: contributor

I’m with @jgarzik on this. We shouldn’t extend “pure function” calls. Bitcoin-tx can handle locktime modification already. @dgenr8: For RPC tests i would recommend to fiddle with the hex-/byte-stream to change the lock_time’s uint32_t. Isn’t it always at the end of the serialized data? Or you can follow @sipa’s advice and use bitcoin-tx within a rpc tests (this is possible already through some python shell exec/piping). @petertodd: Adding a bitcoin library for tests would be possible however i think it would be a overkill and tests-environments should be lightweight to avoid test result displacement IMO.

so I tend to NACK

dgenr8 commented at 2:35 am on April 8, 2015: contributor

This PR is a bugfix imho since the rawtx API is now incapable of producing a tx that mimics the locked txes that the wallet generates.

If this is closed, I’ll get the job done one of those other ways in #5881. bitcoin-tx requires changes to be able to set nSequence compatibly with the wallet and nSequence also needs fiddling-with in the manual route.

laanwj commented at 10:33 am on April 15, 2015: member

I agree with regard to no longer extending pure-utility functions. On the other hand this is trivial. So weak NACK only.

For RPC tests i would recommend to fiddle with the hex-/byte-stream to change the lock_time’s uint32_t. Isn’t it always at the end of the serialized data?

Yes, I’d prefer to do it in the test code itself too. It’s quite easy to manipulate a transaction from Python. See e.g. https://gist.github.com/laanwj/12b984a838146acd9647 for some transaction and block surgery code I wrote for a test a while ago, but haven’t got around to integrating.

sipa commented at 10:52 am on April 15, 2015: member

Hmm, it is unfortunate that createrawtransaction is unable to have identically to sendtransaction now. I would consider that a bug, but I’m not sure how to fix it.

An extra field is an option, but has the problems already discussed.

We could make it behave identical to sendtransaction wrt locktime by default, but that is 1) not really in the spirit of createrawtransaction 2) not replicatable in bitcoin-tx due to it not knowing the current height.

in src/rpcrawtransaction.cpp: in 690366fd6e outdated

305@@ -306,9 +306,9 @@ Value listunspent(const Array& params, bool fHelp)
306 
307 Value createrawtransaction(const Array& params, bool fHelp)
308 {
309-    if (fHelp || params.size() != 2)
310+    if (fHelp || params.size() < 2 || params.size() > 3)
311         throw runtime_error(
312-            "createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...}\n"
313+            "createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...} locktime\n"

luke-jr commented at 2:08 am on June 2, 2015:

This fails to express that locktime is optional.

luke-jr commented at 2:13 am on June 2, 2015: member

ut implementation OK (although I agree it is better not to extend utility RPC in principle)

dgenr8 force-pushed on Jun 2, 2015

dgenr8 commented at 5:42 am on June 2, 2015: contributor

Updated 1-line help to indicate locktime is optional. Thanks Luke.

Add optional locktime to createrawtransaction

A non-zero locktime also causes input sequences to be set to
non-max, activating the locktime.

212bcca920

dgenr8 force-pushed on Aug 10, 2015

jgarzik commented at 6:15 pm on September 15, 2015: contributor

Closing this. “The wind is blowing negative” based on comments, and the long term direction is to remove pure function RPC calls; you don’t need to call a server to achieve what is better left to a lib or bitcoin-tx. Just as easy to use bitcoin-tx as it is to use bitcoin-cli from the command line. And if it’s custom code, just use a lib.

jgarzik closed this on Sep 15, 2015

dgenr8 commented at 6:46 pm on September 15, 2015: contributor

Recap:

@gmaxwell Was concerned that future rawtx RPC upgrades are so important, this one should wait for them
Others stated that rawtx RPC upgrades are not only unimportant, but are discouraged
In spite of this, rawtx upgrade #6346 was happily merged

Conclusion: [ERROR] internal consistency failure

jgarzik commented at 6:49 pm on September 15, 2015: contributor

Happy to re-open this if people think it will get merged in the short term. “easy to close, easy to reopen” is the more general goal.

luke-jr commented at 0:40 am on September 20, 2015: member

IMO it makes more sense to merge this than #6346 did

dcousens commented at 2:04 am on September 20, 2015: contributor

Is there a document referencing general direction for these things? If the above is true (ref @dgenr8’s comments), then, why did #6346 get merged?

laanwj commented at 10:36 am on September 27, 2015: member

Reopnening this. Even though I’d like to move away from internal utility functions as well, it has only very local code impact but there are a lot of people asking for it, so may as well merge it in the “doesn’t hurt” category.

laanwj reopened this on Sep 27, 2015

laanwj merged this on Oct 23, 2015

laanwj closed this on Oct 23, 2015

laanwj referenced this in commit bf7c1958d1 on Oct 23, 2015

zkbot referenced this in commit 3b0a5bcd24 on Apr 13, 2018

zkbot referenced this in commit 65a8f9f201 on Apr 13, 2018

dgenr8 deleted the branch on Sep 19, 2018

DrahtBot locked this on Sep 8, 2021

[RPC] Add optional locktime to createrawtransaction #5936