Skip to content

Fix JSON parameter parsing in warnet bitcoin rpc command #725

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

b-l-u-e
Copy link
Contributor

@b-l-u-e b-l-u-e commented Jun 23, 2025

Description

This PR addresses issue #714 by implementing JSON parameter handling for the warnet bitcoin rpc command. Currently, the command fails when users attempt to pass JSON parameters due to missing parameter processing logic.

Problem Analysis

The issue manifests when users attempt to pass JSON parameters to Bitcoin Core RPC methods:

$ warnet bitcoin rpc tank-0000 getblock '["146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805"]' 'true'
error code: -8
error message: blockhash must be of length 64 (not 66, for '[146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805]')
command terminated with exit code 8

Root Cause: The current _rpc() function has incorrect parameter handling:

  1. Function signature expects params: str but should be params: List[str]
  2. No JSON parameter processing logic exists
  3. Shell parsing breaks JSON arrays into separate arguments
  4. Bitcoin Core receives malformed input like [146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805] instead of the actual hash

Technical Solution

Parameter Type Fix

  • Function Signature: Change params: str to params: List[str] to properly handle multiple arguments
  • Argument Processing: Add proper parameter reconstruction and processing logic

JSON Reconstruction Logic

Add _reconstruct_json_params() function that:

  1. Detects Split JSON: Identifies when JSON structures are fragmented across multiple arguments
  2. Reconstructs Parameters: Joins split JSON parts back together
  3. Quote Restoration: Adds quotes to unquoted string arrays ([network]["network"])
  4. Validation: Ensures reconstructed JSON is syntactically valid

RPC Parameter Processing

Enhance _rpc() function with:

  • JSON Array Handling: Properly process JSON arrays for bitcoin-cli
  • Type Preservation: Handle boolean, numeric, and string values correctly
  • Backward Compatibility: Maintain support for non-JSON parameters

Verification

Current Behavior (Broken)

$ warnet bitcoin rpc tank-0000 getblock '["146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805"]' 'true'
error code: -8
error message: blockhash must be of length 64 (not 66, for '[146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805]')

Expected Behavior (After Fix)

$ warnet bitcoin rpc tank-0000 getblock '["146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805"]' 'true'
{
  "hash": "146cc34515ae4ad2e938c4b8a899ac2889b37f603e9bba51854eb14540250805",
  "confirmations": 1,
  ...
}

Impact

This fix will enable:

  • Proper JSON Parameter Handling: Support for arrays, objects, and complex parameters
  • Bitcoin Core RPC Compatibility: Correct parameter formatting for bitcoin-cli
  • Shell Safety: Robust handling of shell argument parsing edge cases
  • Backward Compatibility: Existing non-JSON RPC calls continue to work

Files Modified

  • src/warnet/bitcoin.py: Fix parameter type and add JSON reconstruction logic

Fixes #714

@b-l-u-e b-l-u-e force-pushed the fix-json-parameter-parsing branch 2 times, most recently from 5b3383a to 8557356 Compare June 23, 2025 16:07
@pinheadmz
Copy link
Contributor

Expected this to work:

 --> warnet bitcoin rpc tank-0001 logging '["http","rpc"]'
error: Error parsing JSON: http
command terminated with exit code 1

It did work with slashes:
warnet bitcoin rpc tank-0001 logging '[\"http\"]'

@pinheadmz
Copy link
Contributor

pinheadmz commented Jun 26, 2025

Another thing that may be out of scope for this PR is there can be a swallowed error
bash: line 1: /usr/local/bin/kubectl: Argument list too long

for example this beast, which I'd love to see working at some point:

warnet bitcoin rpc tank-0001 importdescriptors '"descriptors":[{"desc":"pkh(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/44h/1h/0h/0/)#ulcldqpa","timestamp":1750949660,"active":true,"internal":false,"range":[0,999],"next":0,"next_index":0},{"desc":"pkh(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/44h/1h/0h/1/)#dta7s439","timestamp":1750949660,"active":true,"internal":true,"range":[0,999],"next":0,"next_index":0},{"desc":"sh(wpkh(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/49h/1h/0h/0/))#ygm43e8l","timestamp":1750949660,"active":true,"internal":false,"range":[0,999],"next":0,"next_index":0},{"desc":"sh(wpkh(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/49h/1h/0h/1/))#ztns25vt","timestamp":1750949660,"active":true,"internal":true,"range":[0,999],"next":0,"next_index":0},{"desc":"tr(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/86h/1h/0h/0/)#6pett65n","timestamp":1750949660,"active":true,"internal":false,"range":[0,999],"next":0,"next_index":0},{"desc":"tr(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/86h/1h/0h/1/)#t4u2k0yt","timestamp":1750949660,"active":true,"internal":true,"range":[0,999],"next":0,"next_index":0},{"desc":"wpkh(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/84h/1h/0h/0/)#8rwp50x9","timestamp":1750949660,"active":true,"internal":false,"range":[0,1000],"next":1,"next_index":1},{"desc":"wpkh(tprv8ZgxMBicQKsPdDAbWAtdMmeLt8H9t8aTwHARZ2paraAf3D167mWwqvxUcpmFmPb538Ymepz1cPLjiwzGkR7ep7yxFATw4sX2BFmqsPJYnoW/84h/1h/0h/1/)#khtqf6ka","timestamp":1750949660,"active":true,"internal":true,"range":[0,999],"next":0,"next_index": 0}]'

@@ -346,3 +384,96 @@ def to_jsonable(obj: str):
return obj.hex()
else:
return obj


def _reconstruct_json_params(params: list[str]) -> list[str]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a lot of work, implementing our own JSON parsing? Did you try any other approaches? Is there a better way to get the raw input string from click perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i will adjust the changes..thank you for the review

@pinheadmz
Copy link
Contributor

I cherry-picked the lint fix to main, if you wanna rebase to run ci

- Add _reconstruct_json_params() function to handle JSON parameters split by shell parsing
- Update _rpc() to properly process JSON arrays and primitive values for bitcoin-cli
- Fix issue where shell parsing would break JSON parameters into separate arguments
- Handle edge cases like unquoted string arrays [network] -> [network]
- Maintain backward compatibility with non-JSON parameters

This fixes the issue where commands like:
  warnet bitcoin rpc tank-0000 getnetmsgstats '[network]'
would fail due to shell parsing breaking the JSON array.

Fixes bitcoin-dev-project#714
@b-l-u-e b-l-u-e force-pushed the fix-json-parameter-parsing branch from fb816d4 to 86c22e6 Compare July 2, 2025 21:58
- Accepts JSON params with or without backslash-escaped quotes (e.g. '[http]' and '["http"]')
- Wraps JSON params in single quotes for correct shell and bitcoin-cli parsing
@b-l-u-e b-l-u-e force-pushed the fix-json-parameter-parsing branch from 7d22984 to 8449337 Compare July 2, 2025 23:36
@b-l-u-e
Copy link
Contributor Author

b-l-u-e commented Jul 2, 2025

The following is fixed:

  • JSON arrays for logging: '["http","rpc"]'
  • JSON arrays for getblock: '["blockhash"]' → extracts blockhash
  • Plain string parameters still work: getblock 'blockhash'
  • Empty arrays handled correctly

…hods

- Fix getblock '[blockhash]' issue by extracting first element from JSON arrays
- Maintain backward compatibility for plain string parameters
- Support JSON arrays as-is for methods like logging and importdescriptors
- Extract only first element for single-parameter methods (getblock, getblockhash, gettransaction)
- Extract all elements for multi-parameter methods
- Fix malformed JSON patterns with escaped quotes
@b-l-u-e b-l-u-e requested a review from pinheadmz July 3, 2025 00:06
@b-l-u-e
Copy link
Contributor Author

b-l-u-e commented Jul 3, 2025

#725 (comment)

For the long importdescriptors command you mentioned, a few potential solutions could be explored in a future PR:
maybe file-based approach writing the descriptor data to a temporary file and pass the file path instead of the raw JSON
or perhaps chunking like split large descriptor arrays into smaller batches or use a different method to pass large data (like environment variables or stdin)

@pinheadmz
Copy link
Contributor

I still don't think we need to maintain our own JSON parsing code like this. I think the issue in #714 is that @click.argument("params", type=str, nargs=-1) returns a List of what it thinks are separate arguments separated by spaces. I'm wondering if its easier to just get sys.argv and try to pass all the bitcoin-cli args as one single string. The commands are executed inside the pod itself , currently like f"bitcoin-cli {method} {params}" so we should be able to rely on bitcoin-cli interpreting the argument string correctly, as long as it is sent to the shell inside the pod correctly.

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.

warnet bitcoin rpc should handle JSON params better
2 participants