Improvements to the new RPC. #292
Comments
|
Some of these, generally the RPC routes, do need to be tested before launching. |
kayabaNerve
added a commit
that referenced
this issue
Mar 12, 2021
kayabaNerve
added
feature
|
Another casualty is the multi-account support of the personal RPC (not the multi-address support; referring to BIP 44). It's not needed and greatly simplifies the APIs. In the future, they can be added via optional fields. Also removing hardened address generation. |
kayabaNerve
added a commit
that referenced
this issue
Mar 13, 2021
|
personal_getTransactionHistory will be needed for the Wallet GUI. personal_getBalance to compliment transactions_getBalance as personal_getUTXOs compliments transactions_getUTXOs would be great. |
kayabaNerve
added a commit
that referenced
this issue
Apr 28, 2021
Slower, yet enables getting the full history of a key's interactions. Will be useful in the planned personal_getTransactionHistory mentioned in #292. Updates the broken spendable test to the latest policies. It now passes.
kayabaNerve
added a commit
that referenced
this issue
Apr 29, 2021
Doesn't expand it to match the revamp done as part of this branch; does a minimal addition of equality checks and adds a note it should be expanded. #292 takes that burden.
|
The above commit adds a task of expanding the WalletDB test as well. |
|
Some of the following should've been done as part of the main PR. That said, it's dragged on too long, and I really just want to merge it. None of these need to be done, from a technical perspective, and they do generally fall under our extremely comprehensive commentary about testing.
wasn't written. The last one is actually somewhat important; Meros won't handle this edge case. A UTXO mutated in such a way won't become present on step 4 under the current codebase. |
kayabaNerve
added a commit
that referenced
this issue
Apr 29, 2021
* New RPC docs. Specifies HTTP, digest auth, switches over to using objects as parameters, removes dated methods, removes complex and niche methods, corrects handling of BIP 32... * Port the Transactions/Consensus/Merit/Network RPC modules. Without the matching macro, this is mostly meaningless. Updates the docs accordingly. Notably removed is the ability to broadcast DataDiffs/SendDiffs. This is due to the fact they're ordered and sequential, and at the same time, the protocol doesn't alloow requesting specific Elems. This means broadcasting Elem 5 will forever be doomed if Elem 4 didn't propagate. * Have the Wallet mnemonic affect both the MinerWallet and Wallet. Simply a documentation edit. * Add watch wallet/offline signing RPC routes. Also adds a note on the behavior of network_connect. * Redo the RPC macro/supporting code. * Re-enable the Merit RPC. Previous commit disabled all but Transactions/Network (and technically system which doesn't actually exist as a module). This re-enables Merit, leaving Consensus/Personal. * Fix multiple modules/replying. Misplaced break/reply wasn't called/reply didn't provide the surrounding object. * Add requireAuth Previously commented; now not in order to ensure it's not forgotten. The GUI is always authed; the HTTP RPC's authed status is currently hardcoded to true. Moves system_quit to its own module in order to enable access to requireAuth, a simulated pragma. While an auth check could've been used where it was originally, removing hardcoded methods is beneficial. Correctly extends the RPC macro with requireAuth, and enables directly taking the request/reply function as arguments. Adds a missing method not found error. Would previously only trigger if the module was found and the method wasn't. Corrects the reply function to not supply the outer object, yet rather the macro generated reply call (as having it in the reply function itself broke errors). * Remove unnecessary imports. Also remove exports, adding missing imports in the relevant files. Also includes a set of lines met for the previous commit. * Don't expect work to be performed by signTransactionTemplate. As one of the fields is named `type`, a keyword, the RPC macro will now handle arguments defined with "_JSON" as fields without such a string. Adds needed length checks to the Claim/Send/Data parsers. * Clarify and clean RPC docs a bit. * Re-enable Consensus/Transactions. * Misc tweaks. newException -> newLoggedException. Majority are in relevant code. Some are from master. Removes unused import when compiling with nogui (threadpool). * Misc tweaks. newException -> newLoggedException. Majority are in relevant code. Some are from master. Removes unused import when compiling with nogui (threadpool). * Stub out missing RPC routes. Solely personal has work to do. Also removes signTransactionTemplate; an independent binary without the bloat of the node should do that. * Tweaks. Adds a getMeritHolderNick route to the personal module. Fixes a bug in parsing BLSPublicKeys from JSON (length checked against the hex length despite already parsing out of hex). * Update the Python tests to the new RPC. The two methods calling personal_data no longer do so, as personal_data isn't impolemented yet. Also cleans NodeThresholdTest a bit, correcting a name differing from its file. The one test using personal_send (AddressTest) still fails due to the unimplemented nature of the RPC route. * Move the Address test off personal_send. Now all Python RPC tests should pass. * Stub out Python test files. Some functions share tests, due to scope (consensus_getSendDifficulty + consensus_getDataDifficulty). Such test files have the list of routes they're intended to test written out. Every test has an empty string at the top in order to trigger Pylint. Ensures that they won't be missed. * Stub tests expecting specific HTTP errors. * Update the Mint RPC definition Corrects the definition as the existing one was invalid, and updates it The Python JSON impl is also updated, yet that was never used in the vectors, hence the lack of regen * Stub test for invalid non-Nim-native RPC types * Remove argon from getTransaction. In response to #289. * Add a test for GetTransaction. Correct Python's JSON serialization in relation to the RPC edits made for #289, which already wasn't used when loading the JSON back into Python. * Stub a test for integer bounds. * Test publishTransaction. Fixes a bug in publishTransactionWithoutWork. * Remove stray print. * Test getSendDifficulty/getDataDifficulty. * Test getBlockTemplate. Closes #278. Fixes a bug where old templates were always dropped, even if they were for the current block (part of #278). Fixes a crash where the RPC took in a header of indescriminate length, which parseBlockHeader didn't check. * Augment getBlockTemplate with a MeritRemoval inclusion test. * Augment publishTransaction's test with broadcast behavior checks. * Change merit_getBlock's id field to block. * Tweaks to Merit RPC docs. No functional changes. * Test getBlock. Adds the packets field to the JSON, as Python needs it. While it was possible to remove it from Python, which was considered as this field has been previously excluded for being of little value, it is part of the header. If we ever add a getBlockHeader function, its value greatly increases. Fixes an oversight on the range checks in retrieveFromJSON. * Remove a test's expectation packets wasn't in Meros's JSON for a Block. Missed byproduct of the last commit. * Misc cleanup. * Rename RPC HTTP tests. Adds chunked encoding test. * Rough HTTP code. Closer to scribbles than code. Does a lot of HTTP work; more than's needed. The existing HTTP error code tests should now be all that are used * Working HTTP draft. Works against curl. Has no where near the functionality it should. Further stubs tests. * Update Python to use HTTP. * Update existing tests for #278. * Correct default HTTP socket close behavior. It's not keep-alive by default. Also removes the easily worked around 100 requests per second rate limit. * Support authorization. Updates the RPC and config object. Does not update Python except the system_quit call. Also doesn't bother to spawn the RPC when it and the GUI are disabled. * Correct Connection header parsing. I assumed it'd just be keep-alive. It's actually a comma separated list which is generally just keep-alive. Moves to a default policy of close. While that was the case for 1.0, 1.1 defaults to keep-alive, when Meros expected close. While keep-alive will be supported, it's more efficient from a FD perspective to regain that slot ASAP. Also utilizes a pair of logWarn statements for exceptions when shutting down. * Clean the HTTP code and re-enable headers * Have Python auth against the RPC by default * Add missing auth token argument to 232's custom process spawn. * Test auth requirement in every standing RPC test. Also provides an improvement to the GetBlockTemplate test. * Basic batch request support. Still needs further testing. Prelim req to chunked encoding, AKA the reason I'm moving on for now. * Test chunked encoding. Tests it using batched reqs, which isn't actually a requirement (no idea why I thought it would be), yet this the only usage that makes sense AFAIK. Some chunks are not valid JSON on their own, so the test stands as valid. * Support chunked encoding. Meant for the last commit. * Fix 100-continue support. Also handles the body no matter the content type (as curl defaults to x-www-form-urlencoded). Increases documentation. * Remove expectation of spaces in header definitions. * Multiple bug fixes + cleanup. Fixes Authorization, which broke in the previous commit. Returns HTTP 417 instead of 400 if the message body is too long. Fixes support for HTTP 100. Fixes readLine, as the version shipped with chronos only support \r\n. THe new version supports all three. Corrects RPC recv to not raise. Tests HTTP 100 support. * Test and fix support for all newline types. * Drop HTTP pipelining. * Drop GET and HEAD. Technically breaks HTTP compliance, yet they're not needed and should never come up. Not worth having. * Stub support for Range headers. Just acknowledges their existence and relevance. Our response would just be a 416. * Remove aspects/tests no longer required to be done before merging. See #292. * Test authorization. * Fix #162. Start of the new personal RPC. Provides a non-compliant personal_data which can be used as the starting point. * Add a pair of lines meant for the last commit. * Remove multi-Account functionality from the spec. See #292. * Work on testing seed management. * Update the Multiline Expansion Checker. False handling of a variable named defData. * Add warnings about the BIP 32 implementation. * Implement new seed management. Fixes #293. Tests seed handling/HD derivation/several RPC routes, yet the Seed Test isn't complete; it still needs to test addresses (included as they're derived from the seed; due to their scope, it may best to use its own test, as originally planned). Also lints the public domain ed25519 implementation added. * Implement HD public key derivation. * Implement getAddress for specific address. Also updates personal_data to handle hex data and the arguments of getTransactionTemplate. * Theoretically implement getAddress. Further extends SeedTest. * Extend the seed test with getAddress (without arguments; unused) checks. Uses the ed25519 RFC's signature code, ported to the existing ref impl's internal functions and cleaned up a bit (done due to an unknown issue with the original). Slightly edits the private key management code of BLS.py in how it converts a 32-byte to a 48-byte value. Randomly complained it couldn't rjust with a byte char, despite working in the past and in my local python console. Deletes the independent getAddress test for being irrelevant. * Rename setMnemonic to setWallet. It requires the password as well. Originally named setMnemonic as the password isn't saved, so it only sets the Mnemonic in the DB, as well as for parity with getMnemonic. * Misc improvements to the SeedTest. * Correct key handling for Datas. Adds a note to SeedTest to test it. Also fixes an argument name in the personal_data RPC route. * Rename the setPublicKey impl. Still not functional. Just a housekeeping task. * Finish SeedTest's actual seed test sections. Doesn't test the relevant RPC routes have the proper access lock, nor other invalid cases. Does finish testing all expected behavior for seed/address management. * Test invalid RPC calls related to seed management Includes a Nim bug fix meant for the last commit Also includes a new requirement all mnemonics must have 24 words (256 bits of entropy). Previously, they were generated as such, yet you could set one of any size. * Split up and clean the Seed test It's now Derivation and GetAddress test, with the latter also handling getMeritHolderNick and consistency when rebooting/loading the wallet. * Finish batch testing. Fixes bugs in how they were handled, as well as discovers a DoS vector where non-object requests caused a panic when used as the objects they're expected to be. They should be fixed now, yet a new test has been added to test them exclusively. Also removes a dated error case claiming we don't support batch requests. Also fixes a deprecation warning in the RPC folder, yet not other instances through out the codebase. * Test the Data RPC route. Also includes a lint fix for the batch test. * Test invalid request objects. * Misc lint. Also adds back the finalized property to getStatus. * Variety of getUTXOs test. 3 are complete, 1 is in progress, 2 are solely described. Fixes getStatus's re-added finalized field. * Finish the getUTXOs reorg test. * getUTXOs immediately spent test. Also fixes minor type errors in the reorg test. * Test broadcast. * Clean the GetUTXOs tests. Also includes a pair of minor tweaks to the Broadcast test. * Address discovery when reloading seeds (prototype). * Implement personal_getUTXOs. Test it and address recovery. * Delete the JSONRPC test. The error cases are already tested by the InvalidRequest test. The success cases are implicitly tested everywhere, and RPC.py has been expanded to properly check the jsonrpc/ID field. * Integer bound test. Corrects when the RPC should check the response's ID. Fixes a bug in boundary checking where a number outside of the uint16 range was accepted as a uint16. * getAccountKey -> getAccount. Also returns the chain code which is needed for further derivation. * Correct BIP 44 derivation. We swapped the internal/external account chains. 0, AKA change = false, is external. 1, AKA change = true, is internal. Also adds a getter for change address public keys to the Personal tests' Lib file + shifts function boundaries a bit. * WatchWallet support. * Fix creating Claims for Mints to the node's Wallet. Previously used the older API to get what key to mint to. Development had occurred by commenting out this block. Now uses the new API. Always mints to the first valid address on the wallet in order to not use further keys, as there's no added anonimity benefits given they can be linked to a single Merit Holder key. * Update usage of MuSig. Now formalized, using all keys (not just unique keys) for closer spec accuracy, a H of Blake2b-512, and a DST for Hagg. Adds a Python implementation. Cleans the code which declares Blake2b instances. * Change account chain management. * Misc fixes to WatchWallet functionality. * Tweaks. Has HTTP validate the range of the content length. Removes unused exports from Util. Has removeFromSpendable remove all occurrences, not just the first; stops edge cases where the same UTXO is reported as spendable twice. Also has a trade off on performance (more expensive generally, significantly less expensive rarely). Updates one test to have an empty string statement so pylint flags it. Updates another with a note on something it has to do. * Implement private key aggregation to match a MuSig public key. Doesn't implement MuSig, which is secure for MPC, yet rather a naive version that works given full knowledge of all private keys. * Use KeyIndex; a struct for the change bool and index uint32. Also documents the scalar -> extended private key code when aggregating private keys. * Generator for the WatchWallet test. Extremely subject to change. * Have EdPrivateKey aggregate not aggregate when there's a single unique key. * getTransactionTemplate prototype. * Rewrite personal_send to wrap personal_getTransactionTemplate. p_gTT does all the required logic/formatting for UTXO selection and output generation. There's no reason to duplicate it. This does parse the JSON output though, so moving the core logic to its own function, making p_gTT just a `%*` call, and making personal_send not call another RPC function, would probably be optimal. personal_send previously wouldn't compile/was disabled, so this is a major step forward. Tests for both p_gTT and personal_send coming soon. * Ed25519 aggregation fixes. Cleans up the unique key check; fixes a bug where the first key was included twice when adding a Send. * Test personal_send. * Test every personal method requires auth. Previously, some methods weren't checked as they weren't implemented. Not that all are... Requires moving the auth check to earlier in the function, which is a good thing in a couple of ways. Doesn't waste time arg parsing if not authed/less chance for something which changes data to happen. * Fix change address recovery. The wrong account chain was used. I have no idea how this was undetected for so long. The Personal Send test should've caught this. * Shift error codes so none use 0. NotEnoughMeros, a status which therefore used a positive error code (or rather, a non-negative error code), used 0. As 0 generally means success, it has been moved to 1. Shifts Spam to 2. * Add change specification to getTransactionTemplate. Functionality was previously missing. Also adds errors for no outputs/0 value outputs. * WatchWallet test. * Test transactions_getBalance. Just hooks into the transactions_getUTXOs test. * Test parsing of string based RPC types. Also removes the GetPeersTest to be done later. * Test fixes. They still don't pass under the most recent changes, yet they now compile and have some runtime fixes. * Support hex strings prefixed with "0x" in the RPC. * Don't remove spent UTXOs from spendable. Slower, yet enables getting the full history of a key's interactions. Will be useful in the planned personal_getTransactionHistory mentioned in #292. Updates the broken spendable test to the latest policies. It now passes. * Fix Data detection when loading an account. Apparently, the fix for #162 broke at some point; the WalletDB used the first address under account 0, yet it checked the Datas for account 0 proper. * Fix the TGU generator. Verified the old Claim and the new Claim in the same Block; didn't generate enough Blocks for a reorg. Remove a dated requirement to add back to spendable (since we now keep in spendable). Expands the spendable test a bit. * Tweak the WalletDB test. Doesn't expand it to match the revamp done as part of this branch; does a minimal addition of equality checks and adds a note it should be expanded. #292 takes that burden. * Case insensitive header support. The HTTP spec doesn't require capitalization to match, and some libraries use all lower case forms. * Fix the return value for merit_publishBlock. * Also test getMeritHolderKey's WatchWallet behavior. * Misc tweaks in preparation for merging. * Delete the samples folder. The RPC Sample should be, and with this commit will be, deprecated for curl at this point. The DB dump sample is irrelevant when we can just share a data folder and go from there. Sure, it's not in a JSON format, but it's easy enough to spin up a node and run the relevant commands as needed. * Remove stray file. * Replace usage of hardcoded lengths with properly generated lengths. I have no idea why these made it in the first place. * Correct oversight in the Spendable test. It assumed an output's destination had outputs considered spendable despite not verifying all Transaction. Adds another safety check that may or may not be needed to ensure a lack of such crashes. * Restore the samples folder. * Correct the help stament for the RPC sample. We abstracted field names into the historical array structure via providing an ordering.
|
merit_getSyncProgress would also be a useful RPC route, notably for the GUI, When syncing to the point we need a block list, we can set known chain length to the result of the list BEFORE verifying the headers, yet also noting the chain as "untrusted" until AFTER we verify them. Current progress is defined as how many block bodies we've processed. |
|
Should likely be network_getSyncProgress. |
kayabaNerve
added a commit
that referenced
this issue
May 30, 2021
This should NOT be used by services as it's currently inaccurate. While it includes payments in with the same threshold as getUTXOs, it has a much looser stance on payments out (where it'll naively select a 'best option' from unconfirmed options). While this works for the GUI, it is not up to spec for what services need, which is clearly documented in its definition. On a branch as it's untested and needed by the new GUI (not a blocker but I would like to wait and see it used in the new GUI). Has implications for #292.
|
The above getTransactionHistory is untested; the Personal Send test would be great to shim its test into. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The new RPC (#279) is having planned features removed in order for it to continue in a timely fashion.
The most notable is the lack of HTTP pipelining. That said, we also have a lack of spec compliance thanks to a lack of GET/HEAD support (just returning a 404). We arguably shouldn't bother with the 404, yet I do want it noted.
Finally, there's a lack of testing, mainly due to the implicitness/irrelevance of a lot of this stuff.
Untested HTTP errors:
Un-explicitly tested RPC routes:
I also want to add in a RPC method,
merit_getSupply, to get the current Meros supply. It'd be defined as:Notable similarities with #272.
The text was updated successfully, but these errors were encountered: