New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RPC] Add optional locktime to createrawtransaction #5936

Merged
merged 1 commit into from Oct 23, 2015

Conversation

Projects
None yet
9 participants
@dgenr8
Contributor

dgenr8 commented Mar 22, 2015

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 22, 2015

Member

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.

Member

gmaxwell commented Mar 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Mar 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.

Contributor

dgenr8 commented Mar 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 22, 2015

Member

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.

Member

gmaxwell commented Mar 22, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 23, 2015

Member

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

Member

sipa commented Mar 23, 2015

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

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Mar 23, 2015

Contributor

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

  1. bitcoin-tx can already do this.

  2. The "pure function" RPCs are on the road to removal long term. We should not be extending pure-function RPCs.

Contributor

jgarzik commented Mar 23, 2015

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

  1. bitcoin-tx can already do this.

  2. The "pure function" RPCs are on the road to removal long term. We should not be extending pure-function RPCs.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Mar 23, 2015

Contributor

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

Contributor

dgenr8 commented Mar 23, 2015

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

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Mar 23, 2015

Contributor

Ping #5503, #5524.

Contributor

dgenr8 commented Mar 23, 2015

Ping #5503, #5524.

@dgenr8 dgenr8 changed the title from Add optional locktime to createrawtransaction to [RPC] Add optional locktime to createrawtransaction Mar 23, 2015

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Mar 26, 2015

Contributor

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

Contributor

petertodd commented Mar 26, 2015

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

@jgarzik jgarzik added the RPC/REST/ZMQ label Apr 5, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 7, 2015

Member

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

Member

jonasschnelli commented Apr 7, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Apr 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.

Contributor

dgenr8 commented Apr 8, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 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.

Member

laanwj commented Apr 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 15, 2015

Member
Member

sipa commented Apr 15, 2015

Show outdated Hide outdated src/rpcrawtransaction.cpp
throw runtime_error(
"createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...}\n"
"createrawtransaction [{\"txid\":\"id\",\"vout\":n},...] {\"address\":amount,...} locktime\n"

This comment has been minimized.

@luke-jr

luke-jr Jun 2, 2015

Member

This fails to express that locktime is optional.

@luke-jr

luke-jr Jun 2, 2015

Member

This fails to express that locktime is optional.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 2, 2015

Member

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

Member

luke-jr commented Jun 2, 2015

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

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Jun 2, 2015

Contributor

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

Contributor

dgenr8 commented Jun 2, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 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.

Contributor

jgarzik commented Sep 15, 2015

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 jgarzik closed this Sep 15, 2015

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Sep 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

Contributor

dgenr8 commented Sep 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 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.

Contributor

jgarzik commented Sep 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Sep 20, 2015

Member

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

Member

luke-jr commented Sep 20, 2015

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

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Sep 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?

Contributor

dcousens commented Sep 20, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 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.

Member

laanwj commented Sep 27, 2015

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 laanwj reopened this Sep 27, 2015

@laanwj laanwj merged commit 212bcca into bitcoin:master Oct 23, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Oct 23, 2015

Merge pull request #5936
212bcca Add optional locktime to createrawtransaction (Tom Harding)

@str4d str4d referenced this pull request Mar 14, 2018

Merged

CLI binary improvements #3086

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #3086 - str4d:cli-binary-improvements-1, r=str4d
CLI binary improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5936
- bitcoin/bitcoin#7550
- bitcoin/bitcoin#7989
- bitcoin/bitcoin#7957
- bitcoin/bitcoin#9067
- bitcoin/bitcoin#9220

Excludes any changes that affected the QT code.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #3086 - str4d:cli-binary-improvements-1, r=str4d
CLI binary improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5936
- bitcoin/bitcoin#7550
- bitcoin/bitcoin#7989
- bitcoin/bitcoin#7957
- bitcoin/bitcoin#9067
- bitcoin/bitcoin#9220

Excludes any changes that affected the QT code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment