Skip to content

Conversation

@sje1
Copy link
Contributor

@sje1 sje1 commented Oct 29, 2013

Some expanded docs for the rpc commands...comments? terrible?

Based on the proposal, update the help message of rpc methods

  • strings arguments are in double quotes rather than square brackets
  • numeric arguments have no quotes (and no default value)
  • optional parameters are surrounded by round brackets
  • json arguments are strings but don't use double quotes

Added 3 sections for the details

  • Arguments: lists each argument, it's type, required or not, a default, and a description
  • Result: The method result, with json format if applicable, type, and a description
  • Examples: examples calls using bitcoin-cli and curl for json rpc call

Problems

  • maybe this is too verbose
  • lines might be too long
  • description are not good or complete
  • examples may be too much

Based on the proposal, update the help message of rpc methods
- strings arguments are in double quotes rather than square brackets
- numeric arguments have no quotes (and no default value)
- optional parameters are surrounded by round brackets
- json arguments are strings but don't use double quotes

Added 3 sections for the details
- Arguments: lists each argument, it's type, required or not, a default, and a description
- Result: The method result, with json format if applicable, type, and a description
- Examples: examples calls using bitcoin-cli and curl for json rpc call

Problems
- maybe this is too verbose
- lines might be too long
- description are not good or complete
- examples may be too much
@laanwj
Copy link
Member

laanwj commented Oct 29, 2013

Looks good

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a space here

@laanwj
Copy link
Member

laanwj commented Oct 29, 2013

ACK, this is certainly an improvement over what is there

@sipa
Copy link
Member

sipa commented Oct 29, 2013

There seems to be a lot of duplication (the example command lines in particular). Mind moving the generation of those to a common method?

@sje1
Copy link
Contributor Author

sje1 commented Oct 30, 2013

Thanks for the comments. I have done a few updates.
I had done this on the 0.8.5 branch and tested the output, but haven't been able to figure out how to build the head to verify it (on centos 5)

@BitcoinPullTester
Copy link

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/53b1b1ade8b4d6605fbf145bb62489627cecae68 for binaries and test log.

This could happen for one of several reasons:

  1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
  2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
  3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
  4. The test suite fails on either Linux i386 or Win32
  5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

The examples are generated from a common method (see bitcoinrpc.h).
This also fixes is missing space in rpcblockchain.cpp:101
Some other cleanup
@laanwj
Copy link
Member

laanwj commented Oct 30, 2013

@sje1 what problem do you have building head on centos 5?

@sje1
Copy link
Contributor Author

sje1 commented Oct 31, 2013

i was building 8.0.5 by building each dependency (boost, db, openssl), then setting the paths in the Makefile.

on centos 5, not sure how to update or install the latest autoconf

./autogen.sh
configure.ac:2: error: Autoconf version 2.60 or higher is required

on centos 6, autoconf is ok, i couldn't get it to use my boost but it worked with the built-in one, but key.cpp fails with lots of undefined references

key.cpp:184: undefined reference to `EC_KEY_set_conv_form'

@laanwj
Copy link
Member

laanwj commented Nov 4, 2013

The key.cpp errors could have to do with Centos not including the ecc parts of OpenSSL? I remember there was the same issue with Fedora due to patents. Though this would be nothing new with autoconf.

Edit: But is this ready for merging otherwise?

@sipa
Copy link
Member

sipa commented Nov 4, 2013

Sounds like you're building against a non-ECC OpenSSL build, indeed.

@laanwj
Copy link
Member

laanwj commented Nov 6, 2013

You need to use fully qualified std::string in the .h file, hence the pull tester errors:

In file included from noui.cpp:8:
bitcoinrpc.h:155: error: ‘string’ was not declared in this scope
bitcoinrpc.h:155: error: ‘string’ was not declared in this scope
bitcoinrpc.h:156: error: ‘string’ was not declared in this scope
bitcoinrpc.h:156: error: ‘string’ was not declared in this scope
make[3]: *** [noui.o] Error 1

@laanwj
Copy link
Member

laanwj commented Nov 11, 2013

@sje1 any luck yet?

If not, mind if I take over this pull request and get it merged before other RPC changes?

@laanwj
Copy link
Member

laanwj commented Nov 13, 2013

Continued in #3246

@laanwj laanwj closed this Nov 13, 2013
@sje1
Copy link
Contributor Author

sje1 commented Nov 15, 2013

@laanwj thanks for taking over, i haven't figured out how to build yet :)

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Apr 8, 2020
…tcoin#3184)

* From 2 best sets with the same `nTotal` in ApproximateBestSubset prefer the one with less inputs

* There is no reason to run ApproximateBestSubset again if nMinChange is 0

* Apply review suggestions
Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Apr 8, 2020
…tcoin#3184)

* From 2 best sets with the same `nTotal` in ApproximateBestSubset prefer the one with less inputs

* There is no reason to run ApproximateBestSubset again if nMinChange is 0

* Apply review suggestions
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants