Skip to content

Conversation

@domob1812
Copy link
Member

Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the RPC side. This makes sense, since those have a separate error code anyway through JSON-RPC itself. HTTP error codes should signify actual errors on the HTTP transport side.

Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200. This change makes life easier with those (and makes everything in general more consistent).

See also the discussion for upstream Bitcoin.

@domob1812
Copy link
Member Author

Backported this to 1.1 (as b0aeba3) and 1.2 (as 637b422). For master and closing this, we should wait for the testing infrastructure to be merged upstream through bitcoin/bitcoin#15495 first.

Return HTTP_OK (code 200) also for JSON-RPC calls that fail on the
RPC side.  This makes sense, since those have a separate error code
anyway through JSON-RPC itself.  HTTP error codes should signify
actual errors on the HTTP transport side.

Some JSON-RPC libraries make it hard or impossible to properly extract
the JSON-RPC error code if HTTP returns non-200.  This change makes life
easier with those (and makes everything in general more consistent).

See also the discussion for upstream Bitcoin:
  bitcoin/bitcoin#15381
@domob1812 domob1812 merged commit b5c3195 into master Apr 8, 2019
domob1812 added a commit that referenced this pull request Apr 8, 2019
b5c3195 Return HTTP_OK for RPC errors (Daniel Kraft)

Pull request description:

  Return `HTTP_OK` (code 200) also for JSON-RPC calls that fail on the RPC side.  This makes sense, since those have a separate error code anyway through JSON-RPC itself.  HTTP error codes should signify actual errors on the HTTP transport side.

  Some JSON-RPC libraries make it hard or impossible to properly extract the JSON-RPC error code if HTTP returns non-200.  This change makes life easier with those (and makes everything in general more consistent).

  See also the [discussion for upstream Bitcoin](bitcoin/bitcoin#15381).

ACKs for commit b5c319:

Tree-SHA512: 1190aba2a1eda1bf58fd45dc1e4495ec0722d8768fc36e64bae026d738fcf23e6fa7f590389eaa87460e1307c804f49636ed1263c28e43b3020fafb07ff0f768
@domob1812 domob1812 deleted the http-code branch April 8, 2019 16:44
domob1812 pushed a commit that referenced this pull request Jul 28, 2025
…s own suite

c40dbbb test: Move `script_assets_tests` into its own suite (Hennadii Stepanov)

Pull request description:

  This PR ensures that the `script_assets_tests` test case is explicitly reported as "Skipped" when it is not run, making it clearer when running the test suite with `ctest`:

  - on the master branch @ 9355578:
  ```
  $ env -u DIR_UNIT_TEST_DATA ctest --test-dir build -j 16 -R "^script_"
  Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
  Test project /home/hebasto/git/bitcoin/build
      Start 87: script_tests
      Start 83: script_p2sh_tests
      Start 85: script_segwit_tests
      Start 86: script_standard_tests
      Start 84: script_parse_tests
  1/5 Test #84: script_parse_tests ...............   Passed    0.11 sec
  2/5 Test #86: script_standard_tests ............   Passed    0.11 sec
  3/5 Test #85: script_segwit_tests ..............   Passed    0.12 sec
  4/5 Test #83: script_p2sh_tests ................   Passed    0.12 sec
  5/5 Test #87: script_tests .....................   Passed    0.36 sec

  100% tests passed, 0 tests failed out of 5

  Total Test time (real) =   0.37 sec
  ```
  - with this PR:
  ```
  $ env -u DIR_UNIT_TEST_DATA ctest --test-dir build -j 16 -R "^script_"
  Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
  Test project /home/hebasto/git/bitcoin/build
      Start 83: script_assets_tests
      Start 88: script_tests
      Start 84: script_p2sh_tests
      Start 86: script_segwit_tests
      Start 87: script_standard_tests
      Start 85: script_parse_tests
  1/6 Test #85: script_parse_tests ...............   Passed    0.11 sec
  2/6 Test #83: script_assets_tests ..............***Skipped   0.12 sec
  3/6 Test #86: script_segwit_tests ..............   Passed    0.11 sec
  4/6 Test #87: script_standard_tests ............   Passed    0.11 sec
  5/6 Test #84: script_p2sh_tests ................   Passed    0.12 sec
  6/6 Test #88: script_tests .....................   Passed    0.36 sec

  100% tests passed, 0 tests failed out of 6

  Total Test time (real) =   0.37 sec

  The following tests did not run:
   83 - script_assets_tests (Skipped)
  $ env DIR_UNIT_TEST_DATA=/home/hebasto/git/bitcoin/qa-assets/unit_test_data ctest --test-dir build -j 16 -R "^script_"
  Internal ctest changing into directory: /home/hebasto/git/bitcoin/build
  Test project /home/hebasto/git/bitcoin/build
      Start 83: script_assets_tests
      Start 88: script_tests
      Start 84: script_p2sh_tests
      Start 86: script_segwit_tests
      Start 87: script_standard_tests
      Start 85: script_parse_tests
  1/6 Test #85: script_parse_tests ...............   Passed    0.11 sec
  2/6 Test #87: script_standard_tests ............   Passed    0.11 sec
  3/6 Test #86: script_segwit_tests ..............   Passed    0.11 sec
  4/6 Test #84: script_p2sh_tests ................   Passed    0.12 sec
  5/6 Test #88: script_tests .....................   Passed    0.35 sec
  6/6 Test #83: script_assets_tests ..............   Passed    1.58 sec

  100% tests passed, 0 tests failed out of 6

  Total Test time (real) =   1.58 sec
  ```

ACKs for top commit:
  maflcko:
    re-ACK c40dbbb 👈
  ajtowns:
    ACK c40dbbb
  achow101:
    ACK c40dbbb

Tree-SHA512: 25713e1c3b507b6f2a5fecc7b1ea285a6642b906c248769238a58fc0df48489ac5f7606778f9e3653b407b7f1d06563e1554d04321303b350c80eb888500cc5d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants