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

CLI binary improvements #3086

Merged
merged 12 commits into from Apr 13, 2018

Conversation

@str4d
Contributor

str4d commented Mar 14, 2018

Cherry-picked from the following upstream PRs:

Excludes any changes that affected the QT code.

@str4d str4d added this to the v1.1.0 milestone Mar 14, 2018

@str4d str4d requested review from braddmiller and mdr0id Mar 14, 2018

@str4d

This comment has been minimized.

Contributor

str4d commented Mar 14, 2018

These are some generally useful changes, that also remove merge conflicts for bitcoin/bitcoin#8883, which itself will remove merge conflicts for pulling in the upstream Bech32 PRs for Sapling.

@str4d str4d requested a review from daira Mar 21, 2018

@str4d str4d added the review needed label Mar 28, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented Mar 30, 2018

☔️ The latest upstream changes (presumably #3098) made this pull request unmergeable. Please resolve the merge conflicts.

@str4d

This comment has been minimized.

Contributor

str4d commented Mar 30, 2018

Rebased on master to fix merge conflict.

@daira

daira approved these changes Apr 1, 2018

Some non-blocking comments.

to read extra arguments from standard input, one per line until EOF/Ctrl-D.
For example:
$ echo -e "mysecretcode\n120" | src/zcash-cli -stdin walletpassphrase

This comment has been minimized.

@daira

daira Apr 1, 2018

Contributor

This happens to avoid the process table issue because echo is (usually) a shell builtin, but I think this would more clearly avoid it:

$ src/zcash-cli -stdin walletpassphrase
mysecretcode
120
^D
// This function returns either one of EXIT_ codes when it's expected to stop the process or
// CONTINUE_EXECUTION when it's expected to continue further.
//
static int AppInitRPC(int argc, char* argv[])
{

This comment has been minimized.

@daira

daira Apr 1, 2018

Contributor

Add

static_assert(CONTINUE_EXECUTION != EXIT_FAILURE,
              "CONTINUE_EXECUTION should be different from EXIT_FAILURE");
static_assert(CONTINUE_EXECUTION != EXIT_SUCCESS,
              "CONTINUE_EXECUTION should be different from EXIT_SUCCESS");

(This problem will never occur on POSIX or Windows systems, but could technically occur on other platforms. Note that although EXIT_SUCCESS is equivalent to 0 as the return value from main, it is not necessarily defined to be 0.)

@@ -88,7 +88,7 @@ bool AppInit(int argc, char* argv[])
}
fprintf(stdout, "%s", strUsage.c_str());
return false;
return true;
}

This comment has been minimized.

@daira

daira Apr 1, 2018

Contributor

There's an exit(1) on line 141 that should probably be return false;.

This comment has been minimized.

@daira

daira Apr 1, 2018

Contributor

Oh, looks like that got changed to exit(EXIT_FAILURE). But I still think it should probably be return false, for consistency with other returns in that function.

This comment has been minimized.

@str4d

str4d Apr 13, 2018

Contributor

This will be addressed when we pull in bitcoin/bitcoin#10447 and bitcoin/bitcoin#11511 (which we can do in a subsequent PR).

// extract the optional sequence number
uint32_t nSequenceIn=std::numeric_limits<unsigned int>::max();
if (vStrInputParts.size() > 2)
nSequenceIn = atoi(vStrInputParts[2]);

This comment has been minimized.

@daira

daira Apr 1, 2018

Contributor

Another case of atoi being used on untrusted input; see #2075.

@str4d str4d modified the milestones: v1.1.0, v1.1.1 Apr 4, 2018

@str4d str4d added this to 1.1.1: Upstream Improvements in Release planning Apr 4, 2018

@braddmiller

utACK

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 5, 2018

☔️ The latest upstream changes (presumably #2814) made this pull request unmergeable. Please resolve the merge conflicts.

dgenr8 and others added some commits Mar 22, 2015

Add optional locktime to createrawtransaction
A non-zero locktime also causes input sequences to be set to
non-max, activating the locktime.
rpc: Input-from-stdin mode for bitcoin-cli
Implements #7442 by adding an option `-stdin` which reads
additional arguments from stdin, one per line.

For example

```bash
echo -e "mysecretcode\n120" | src/bitcoin-cli -stdin walletpassphrase
echo -e "walletpassphrase\nmysecretcode\n120" | src/bitcoin-cli -stdin
```
Fix exit codes:
- `--help`, `--version` etc should exit with `0` i.e. no error ("not enough args" case should still trigger an error)
- error reading config file should exit with `1`

Slightly refactor AppInitRPC/AppInitRawTx to return standard exit codes (EXIT_FAILURE/EXIT_SUCCESS) or CONTINUE_EXECUTION (-1)
@str4d

This comment has been minimized.

Contributor

str4d commented Apr 13, 2018

Rebased on master to fix merge conflict, and address some of the non-blocking comments.

@zkbot r+

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 13, 2018

📌 Commit abf5dd3 has been approved by str4d

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 13, 2018

⌛️ Testing commit abf5dd3 with merge 3b0a5bc...

zkbot added a commit 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.

@str4d str4d added this to Complete in Zcashd Team Apr 13, 2018

@str4d

This comment has been minimized.

Contributor

str4d commented Apr 13, 2018

util-test failed because I didn't tweak the test case data for Zcash (binary renaming, and re-prefixing t-addrs). Pushed a trivial commit to fix.

@str4d

This comment has been minimized.

Contributor

str4d commented Apr 13, 2018

@zkbot force

@str4d

This comment has been minimized.

Contributor

str4d commented Apr 13, 2018

@zkbot

This comment has been minimized.

Contributor

zkbot commented Apr 13, 2018

⌛️ Testing commit ee6220c with merge 65a8f9f...

zkbot added a commit 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

This comment has been minimized.

Contributor

zkbot commented Apr 13, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing 65a8f9f to master...

@zkbot zkbot merged commit ee6220c into zcash:master Apr 13, 2018

1 check passed

homu Test successful
Details

@str4d str4d referenced this pull request Apr 13, 2018

Open

Bitcoin Core 0.12.0 #2074

193 of 452 tasks complete

@str4d str4d deleted the str4d:cli-binary-improvements-1 branch Apr 13, 2018

@mdr0id mdr0id moved this from Complete to Released (Merged in Master) in Zcashd Team Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment