bitshares/bitshares-core

BitShares Blockchain node and command-line wallet

bitshares-core's Issues

Test renewal of annual membership

From @theoreticalbts on July 22, 2015 15:10

Create an annual membership, renew it before expiration, make sure the date is pushed back the proper amount regardless of when the renewal occurs.

Then try renewing a lapsed annual membership. Make sure fees have reasonable semantics in the gap between expiry and renewal.

Copied from original issue: cryptonomex/graphene#180

Implement and test virtual operations

From @theoreticalbts on August 19, 2015 17:9

Some big fat TODO's in db_market.cpp here: https://github.com/cryptonomex/graphene/blob/eeeab17477a635fcff7f2d294dff8ecf25449b10/libraries/chain/db_market.cpp#L89-L116

This is basically to generate a virtual cancel op to inform account history listeners that orders have been cancelled due to certain asset management actions.

Copied from original issue: cryptonomex/graphene#250

Audit charging of per-kilobyte fees

From @theoreticalbts on July 31, 2015 15:22

Any operation with potentially unbounded size needs to charge a size fee. Determining whether an operation is variable-size is non-trivial. An operation is variable size if it contains a vector, flat_set or other potentially variable-size element.

Note that this containment may be through any number of intermediate structs. For example, any operation which contains an authority has potentially unbounded size.

We should be able to audit this in a semi-automatic way with logic similar to js_operation_serializer which traverses all operations, and noting any variable-size types found (in any number of sub-structures), allowing us to flag each operation as variable-sized (or not). We will still have to manually specify whether any base type is variable-size or not.

Copied from original issue: cryptonomex/graphene#216

Implement unit test for withdrawing balance belonging to PTS address

From @theoreticalbts on July 22, 2015 15:21

Importing balance using PTS address needs to be tested. Since it is migration related, assigning to @vikramrajkumar

Copied from original issue: cryptonomex/graphene#185

Update CLI cookbook instructions (and fix bugs discovered)

From @bytemaster on July 2, 2015 17:14

The CLI cookbook instructions do not work because they have fallen behind the latest code changes. We need to update the cookbook and verify that following the instructions will work as advertised.

Copied from original issue: cryptonomex/graphene#129

object_database created outside witness_node_data_dir on fresh install

From @theoreticalbts on August 20, 2015 21:47

• Install brand new witness_node
• object_database is created in the wrong place
• Subsequent run doesn't recognize it

Copied from original issue: cryptonomex/graphene#257

Implement unit test for withdraw permission on whitelisted asset

From @theoreticalbts on July 22, 2015 15:29

Specifically the withdraw permission shouldn't allow withdrawing of whitelisted asset when either source or destination account isn't whitelisted.

Copied from original issue: cryptonomex/graphene#188

Review use of libsecp256k1 contexts

From @pmconrad on July 3, 2015 14:58

I stumbled across this bitcoin-core/secp256k1#254 (comment) :

That said, your question makes it clear that the intended use isn't clear. That should definitely be clarified in the header file and perhaps a README. I guess it should list this:

• Do not create/destroy contexts for each call. Their creation is far slower than any library call, and part of their purpose is to reuse precomputed values across calls.
• If you perform signing, you should call context_randomize after initialization of the context, and perhaps on a regularly basis (daily?).
• When calling context_randomize pass in external entropy (perhaps from the operating system, perhaps from user input).
• You are responsible for locking. After initialization, only context_randomize requires exclusive locked, but the caller is responsible for not calling any other functions that use the context during randomization.
• If you do both validation and signing, you can either use a shared context for both, or separate contexts. One reason for using separate contexts if that for signing, calling context_randomize requires an exclusive lock on the context. If you need to do validation at the same time, it may be useful to separate them, so validation is not blocked by context randomization.

Apparently we need to call context_randomize regularly (for signing), with proper synchronization.

Copied from original issue: cryptonomex/graphene#131

Implement unit test for proposed tx already containing some owner / active approvals

From @theoreticalbts on July 22, 2015 15:31

Specifically you should trigger the test in the last two for loops: https://github.com/cryptonomex/graphene/blob/dffc83cca9e710e509c307237a3767b966da2505/libraries/chain/proposal_object.cpp#L57-L60

Copied from original issue: cryptonomex/graphene#189

Filter transactions from get_account_history result based on block number

From @theoreticalbts on August 17, 2015 15:4

Negative block number -> relative to head

Copied from original issue: cryptonomex/graphene#236

Verify that public websocket API does not expose sensitive API calls

From @bytemaster on June 30, 2015 20:54

1. network_api::add_node
2. network_api::get_connected_peers

These calls need to remain available only for authenticated connections.

Copied from original issue: cryptonomex/graphene#118

add build instructions for the different OSes

From @vikramrajkumar on June 25, 2015 16:29

can use the bitshares ones as starting points

Copied from original issue: cryptonomex/graphene#89

If no valid, up to date, price feed then....

From @vikramrajkumar on June 9, 2015 17:40

No new shorts No forced settling No margin calls.

TODO: create test.

Copied from original issue: cryptonomex/graphene#8

Localizable exceptions

From @valzav on July 6, 2015 18:19

It would be nice to have exceptions that we can translate to other languages in GUI.
In order to do this an exception may require to have some code or symbol and parameters, e.g. exception object might look like this:
{
error: "not-valid-address",
param1: "1EDJEW3508324LJDSF908342L",
eng: "Not valid address: %1",
message: "Not valid address: 1EDJEW3508324LJDSF908342L",
stack: ...
}

Copied from original issue: cryptonomex/graphene#135

Better handling of non-broadcast transactions in cli_wallet

From @theoreticalbts on August 20, 2015 21:42

• Allow specifying expiration time in cli_wallet
• If broadcast is false, then place transaction into a local store
• Allow users to inspect the local store and broadcast transactions

The motivation here is to provide some way for cli_wallet users to inspect fees before authorizing the transaction.

Copied from original issue: cryptonomex/graphene#256

Investigate cdd_vesting_policy::on_deposit_vested coverage

From @theoreticalbts on July 22, 2015 16:7

Coverage claims unit tests don't hit cdd_vesting_policy::on_deposit_vested. Why?

Copied from original issue: cryptonomex/graphene#197

Peer is disconnecting us because ... attempting to push a block that is too old

From @abitmore on August 22, 2015 23:19

That's probably why it's hard to get into sync after got out of sync?

2015-08-22T22:20:33 p2p:message read_loop on_closing_connectio ] Peer 45.55.6.216:1776 is disconnecting us because of an error: You offered us a block that we reject as invalid, exception: {"code":10,"name":"assert_exception","message":"Assert Exception","stack":[{"context":{"level":"error","file":"fork_database.cpp","line":69,"method":"_push_block","hostname":"","thread_name":"th_a","timestamp":"2015-08-22T22:20:33"},"format":"item->num > std::max<int64_t>( 0, int64_t(_head->num) - (_max_size) ): attempting to push a block that is too old","data":{"item->num":174057,"head":174075,"max_size":10}},{"context":{"level":"warn","file":"db_block.cpp","line":176,"method":"_push_block","hostname":"","thread_name":"th_a","timestamp":"2015-08-22T22:20:33"},"format":"","data":{"new_block":{"previous":"0002a7e8b9613cd334a36aeccdb1d32460d8fa53","timestamp":"2015-08-22T22:20:14","witness":"1.6.4","next_secret_hash":"a6b9319cdc3465202c6ee6ba20f6fcfb4eb26211","previous_secret":"d879b9f9a141a1f9edc662fad77dcfa068e6562c","transaction_merkle_root":"0000000000000000000000000000000000000000","extensions":[],"witness_signature":"2055c39c86e1dd8acc632ea55d010ce56dd01601106c3367fa5ae6b3d08d35a5567a545a0a23a1bace2bf651c049a2559ba75f8ba87b0e597fe9ca88f113a2804e","transactions":[]}}},{"context":{"level":"warn","file":"application.cpp","line":379,"method":"handle_block","hostname":"","thread_name":"th_a","timestamp":"2015-08-22T22:20:33"},"format":"","data":{"blk_msg":{"block":{"previous":"0002a7e8b9613cd334a36aeccdb1d32460d8fa53","timestamp":"2015-08-22T22:20:14","witness":"1.6.4","next_secret_hash":"a6b9319cdc3465202c6ee6ba20f6fcfb4eb26211","previous_secret":"d879b9f9a141a1f9edc662fad77dcfa068e6562c","transaction_merkle_root":"0000000000000000000000000000000000000000","extensions":[],"witness_signature":"2055c39c86e1dd8acc632ea55d010ce56dd01601106c3367fa5ae6b3d08d35a5567a545a0a23a1bace2bf651c049a2559ba75f8ba87b0e597fe9ca88f113a2804e","transactions":[]},"block_id":"0002a7e934a552405ffc797448a046e0bbf916d9"},"sync_mode":true}}]}                     node.cpp:2666

Copied from original issue: cryptonomex/graphene#262

Implement unit test for get_impacted_accounts()

From @theoreticalbts on July 22, 2015 15:17

The get_impacted_accounts() function needs to be tested with all operation types. This will be an enormous test!

Copied from original issue: cryptonomex/graphene#183

Test core_exchange_rate update logic in update_expired_feeds()

From @theoreticalbts on July 22, 2015 16:8

Specifically, the unit tests need to hit this code: https://github.com/cryptonomex/graphene/blob/d827c5df281eafe43d6d0d00533ea0a0f2643c1c/libraries/chain/db_update.cpp#L256-L260

Copied from original issue: cryptonomex/graphene#198

Implement unit test for checkpoints

From @theoreticalbts on July 22, 2015 15:5

The unit test should create several checkpoint(s) and verify the following:

• It works if you get on the right chain
• It throws at the appropriate checkpoint if you get on the wrong chain

Copied from original issue: cryptonomex/graphene#178

Delegate / Witness Resignation

From @bytemaster on July 7, 2015 19:50

Can they resign, should they be able to, and if so how?

Copied from original issue: cryptonomex/graphene#143

fork_database.hpp: fork_item::invalid is never used

From @bytemaster on August 18, 2015 19:32

The original purpose was to track blocks that get pushed and are then found to be invalid and then to prevent the fork database from accepting any additional blocks on top of that block.

Under BTS 0.x we had some issues where a block would get marked as invalid when it shouldn't have and then we would never be able to switch back to that fork. So there is some risk for marking things as invalid, but in theory the BTS 2 undo architecture shouldn't have the same issues.

Copied from original issue: cryptonomex/graphene#248

[Feature Suggestion] No Fee on User Side with Whitelisted UIA

From @clayop on December 6, 2016 0:15

If a UIA issuer has authority to allow or deny users with whitelisting, a possibility of spam attack can be significantly reduced and easily controlled out. In this case, a UIA issuer may want to remove transaction fees from users to facilitate UIA usage, while the issuer pays the fee through the fee pool.

There is a potential customer in Korea(a local government) and if this feature is implemented, we are going to push it more.

Copied from original issue: cryptonomex/graphene#667

Possible Use of Uninitialized Variable

In this constructor, when the condition is false, the content field of the object won't be initialized, then it will lead to undefined behavior.

Implement unit test for all assert predicates

From @theoreticalbts on July 22, 2015 15:20

All predicates in assert.hpp need to be exercised by unit tests.

Copied from original issue: cryptonomex/graphene#184

Tracking vote counts for votable objects should be a plugin

From @theoreticalbts on August 17, 2015 18:8

Vote counts have no meaning for validation. Thus, c0c36ca should be a plugin.

Copied from original issue: cryptonomex/graphene#239

Add settings for witnesses to disable certain operation types

From @bytemaster on July 1, 2015 21:18

In the event of an bug in certain operations, witnesses need the ability to respond quickly and filter transactions that contain those operations.

Copied from original issue: cryptonomex/graphene#122

Implement unit test for custom_operation

From @theoreticalbts on July 22, 2015 15:45

It doesn't really do anything, but the custom_operation should be tested.

Copied from original issue: cryptonomex/graphene#194

Remove udt4 from FC

I guess it's safe to do so. See steemit/fc@b07f429

Implement unit test for whitelist_markets and blacklist_markets

From @theoreticalbts on July 22, 2015 15:23

The whitelist_markets and blacklist_markets are properties on an asset. But first we must ask, what are the intended semantics of these fields?

Assigning to @bytemaster to answer this question.

Copied from original issue: cryptonomex/graphene#186

Upstream secp256k1-zkp API has changed

From @clayop on August 7, 2015 21:55

Followed instruction in Readme.md

CMake Error in libraries/fc/CMakeLists.txt:
  Imported target "secp256k1" includes non-existent path

    "/home/clayop/graphene/libraries/fc/vendor/secp256k1-zkp/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

Copied from original issue: cryptonomex/graphene#226

database_fixture is ripe for refactoring

From @vikramrajkumar on June 9, 2015 17:44

Here are the problems with database_fixture:

• Reliance on macros makes single stepping through tests difficult with GDB.
• "trx" object being modified by macros makes it effectively a global variable. Functions / macros use trx for both input and output.
• Many methods in .hpp could be moved to .cpp file, speed compiling, iterative development of new database_fixture method doesn't need to recompile every test .cpp file (as long as signature in header doesn't change)
• Poor naming conventions in some places (e.g. genesis_account should be genesis_account_id)

Copied from original issue: cryptonomex/graphene#11

Implement unit test for account_transfer_operation

From @theoreticalbts on July 22, 2015 16:6

The account_transfer_operation needs to be tested as follows:

• Ensure whitelist is cleared
• Ensure authorities are updated

Copied from original issue: cryptonomex/graphene#196

[Feature Suggestion] No Fee on User Side with Whitelisted UIA

If a UIA issuer has authority to allow or deny users with whitelisting, a possibility of spam attack can be significantly reduced and easily controlled out. In this case, a UIA issuer may want to remove transaction fees from users to facilitate UIA usage, while the issuer pays the fee through the fee pool.

There is a potential customer in Korea(a local government) and if this feature is implemented, we are going to push it more.

Note: The original issuance is here

Audit all TODO and FIXME comments

From @vikramrajkumar on June 23, 2015 22:1

Copied from original issue: cryptonomex/graphene#77

Add more extensive invariant checks to unit tests

From @theoreticalbts on June 24, 2015 14:46

We should provide a rigorous mode to check invariants at each op, each tx, or each block. This can be used as a debugging tool on both test cases and live networks.

Copied from original issue: cryptonomex/graphene#82

Implement unit tests for all vesting policies

From @theoreticalbts on July 22, 2015 15:14

In particular it appears no test in chain_test creates a new linear vesting policy.

Copied from original issue: cryptonomex/graphene#181

Implement unit test for witness budget running out

From @theoreticalbts on July 22, 2015 15:37

Make sure there is reasonable behavior if witness budget is equal to zero. This unit test may need to artificially change the block interval or witness budget variables by directly affecting the database, as this condition may not be possible in the real blockchain.

This may already be implemented somewhere...

Copied from original issue: cryptonomex/graphene#190

Test worker pay computation when maintenance interval is not 1 day

From @theoreticalbts on July 22, 2015 15:42

Specifically, https://github.com/cryptonomex/graphene/blob/dffc83cca9e710e509c307237a3767b966da2505/libraries/chain/db_maint.cpp#L110-L116 should be covered by this test.

Copied from original issue: cryptonomex/graphene#191

Witness private key may be lost if wallet crashes

From @theoreticalbts on August 20, 2015 22:9

cli_wallet does not save the private key for a new witness until the witness is registered on chain; this is wrong.

Copied from original issue: cryptonomex/graphene#259

Refactor API calls to be more future-proof

From @theoreticalbts on August 18, 2015 16:21

Implementation of cryptonomex/graphene#236 has revealed a general issue when extending API calls. When adding parameters to an API call, it has been noted that we should have a cycle of introducing a new call, deprecating the old call, waiting for web UI code to be updated, and then removing the old call.

The deprecation cycle is necessary because adding parameters to an API call breaks every API client that uses that call, even if the new parameters have default values, because the FC reflection API does not understand how to interact with default values due to underlying deficiencies in C++

In this ticket, I propose a solution to this problem: Most or all API calls should be rewritten to take a single struct, containing the values that were previously passed as separate arguments. Defaults can then be placed in the C++ struct definition, and if some fields do not occur in the JSON dictionary provided by the client, then those fields should not be assigned by the API framework and thus will have their default values.

I should test that the API framework does in fact give fields that do not exist in the JSON object their default values.

Copied from original issue: cryptonomex/graphene#245

Implement unit test to change backing asset of MIA

From @theoreticalbts on July 22, 2015 15:25

MIA's can have their backing asset changed, but only when there is no supply. The unit test should verify:

• Backing asset cannot be changed with nonzero supply
• After the backing asset is changed, it can no longer be borrowed against the old backing
• After the backing asset is changed, it can be borrowed against the new backing

Copied from original issue: cryptonomex/graphene#187

Intermittent connection failure in app_test

From @theoreticalbts on July 14, 2015 22:24

After fixing generally obsolete app_test in cfd9dd0 I notice that every few runs, I get the following error:

$ sudo -u graphene -H tests/app_test --log_level=message
Running 1 test case...
Creating temporary files
Creating and initializing app1
Creating and initializing app2
Starting app1 and waiting 500 ms
1209829ms th_a       application.cpp:190           operator()           ] Initializing database...
1209838ms th_a       application.cpp:63            create_example_genes ] Allocating all stake to 5KQwrPbwdL6PhXujxW37FSSQZ1JiwsST4cqQzDeyXtP79zkvFD3
1209843ms th_a       thread.cpp:95                 thread               ] name:p2p tid:139731355076352
1209845ms th_a       application.cpp:118           reset_p2p_node       ] Configured p2p node to listen on 127.0.0.1:3939
Starting app2 and waiting 500 ms
1210345ms th_a       application.cpp:190           operator()           ] Initializing database...
1210345ms th_a       application.cpp:63            create_example_genes ] Allocating all stake to 5KQwrPbwdL6PhXujxW37FSSQZ1JiwsST4cqQzDeyXtP79zkvFD3
1210349ms th_a       thread.cpp:95                 thread               ] name:p2p tid:139731255420672
1210350ms th_a       application.cpp:107           reset_p2p_node       ] Adding seed node 127.0.0.1:3939
1210351ms th_a       application.cpp:118           reset_p2p_node       ] Configured p2p node to listen on 127.0.0.1:4040
/home/cc/graphene/src/graphene/tests/app/main.cpp(74): fatal error in "two_node_network": critical check app1.p2p_node()->get_connection_count() == 1 failed [0 != 1]
1210852ms th_a       thread.cpp:115                ~thread              ] calling quit() on p2p
1210852ms th_a       thread.cpp:160                quit                 ] destroying boost thread 139731255420672
1210890ms th_a       thread.cpp:115                ~thread              ] calling quit() on p2p
1210890ms th_a       thread.cpp:160                quit                 ] destroying boost thread 139731355076352
1210890ms p2p        thread.cpp:246                exec                 ] thread canceled: 9 canceled_exception: Canceled
cancellation reason: [none given]
    {"reason":"[none given]"}
    p2p  thread_d.hpp:463 start_next_fiber

*** 1 failure detected in test suite "Test Application"

This does not happen on every run, but usually shows up after several runs, if you don't see it after ~10 runs then I would assume your environment is unable to reproduce it.

A similar bug in the past was cryptonomex/graphene#55

Copied from original issue: cryptonomex/graphene#159

Clean up plugin architecture

From @theoreticalbts on August 18, 2015 18:19

Currently our plugin architecture is not well encapsulated. Here are some problems I found:

• We have code like this that manipulates operation_history_object in the core.
• There is no reason to have most_recent_op in account_object as noted in the comment here.
• The annotation subsystem is undocumented, unused and untested, but there are vestigial remnants of an early architecture where it was more widely used, such as meta_account_object.
• The market_history and account_history plugins use the same space ID.
• There is a comment in account_history_plugin.hpp noting that an object ID definition applies to the market history plugin!
• The build scripts contain no provision for compile-time disabling of plugins. We should be able to build a witness_node binary with no account_history_plugin, and in fact should encourage active witnesses to run the stripped-down version without unnecessary indexes.
• The CLI parameters contain no provision for run-time disabling of plugins. You should be able to specify which plugins you want in a config file, to enable/disable plugins without having to recompile.

Copied from original issue: cryptonomex/graphene#246

witness_update operation needs tested

From @theoreticalbts on August 20, 2015 21:59

We need a witness_update_operation to be able to update block_signing_key and url field of a witness object.

Setting the block_signing_key to zero should not be allowed (this will be reserved for when/if we implement witness resignation). There should also be a memo field in the operation which contains the secret, encrypted to the new signing key. The memo field should be vector<char>.

Copied from original issue: cryptonomex/graphene#258

Polish delayed node

From @nathanhourt on August 17, 2015 20:49

Following up on #237, there are some limitations of the initial implementation. Primarily, that the delayed node does not handle forking right now.

Optimally, the delayed node will check that the block ID's match for a given block number between its trusted node and itself, and, if they don't, pop blocks until they do. If it runs out of undo history, it needs to resync (this is currently hard to do without restarting the process, as it's impossible to clear the object graph from memory without destroying the database). Then it will resume syncing from the trusted node as usual.

Copied from original issue: cryptonomex/graphene#240

Implement unit test for "fill call" case in check_call_orders

From @theoreticalbts on July 22, 2015 15:43

The unit test should cover https://github.com/cryptonomex/graphene/blob/dffc83cca9e710e509c307237a3767b966da2505/libraries/chain/db_market.cpp#L439-L446

Copied from original issue: cryptonomex/graphene#192

Implement unit test to verify time of next block

From @theoreticalbts on July 28, 2015 15:9

generate_empty_blocks queries the implementation for the time of the next block. The time of each block (including maintenance intervals) should be verified by some test (perhaps added to generate_empty_blocks, perhaps a new test case).

Copied from original issue: cryptonomex/graphene#208

Implement unit test for MIA backed by asset other than CORE

From @theoreticalbts on July 22, 2015 15:8

Create a unit test for an MIA backed by a non-CORE asset.

• Check that you cannot stack MIA too deep (I think it is a requirement that an MIA is backed by CORE, a UIA, or an MIA backed by CORE).

Copied from original issue: cryptonomex/graphene#179

Regression Tests are not valid until we check for specific exceptions

From @bytemaster on June 30, 2015 18:18

Right now a test checking for an exception will pass regardless of what type of error actually occurred. For the tests to be valid every exception that we test for needs to be explicit.

Copied from original issue: cryptonomex/graphene#113