API get_required_signatures shouldn't return already signed key(s)

[vikramrajkumar]

From @abitmore on March 24, 2016 22:33

The in-code document of get_required_signatures API says:

/**
*  This API will take a partially signed transaction and a set of public keys that the owner has the ability to sign for
*  and return the minimal subset of public keys that should add signatures to the transaction.
*/
set<public_key_type> get_required_signatures( const signed_transaction& trx, const flat_set<public_key_type>& available_keys )const;

If I understood correctly, when an unsigned transaction requires a signature of key A only, sign it with A, then call this API with the signed transaction as trx and A as available_keys, it should return an empty set. However, with latest version (2.0.160316b), the API returns a set contains A. Same behavior is found with earlier versions.

Log of a test on live BTS chain:

> {"id":1, "method":"call", "params":[0,"get_block",[4664932]]}
< {"id":1,"result":{"previous":"00472e6309ca15b5262dedd62e1e5f882fc6ee5e","timestamp":"2016-03-24T22:18:57","witness":"1.6.37","transaction_merkle_root":"68127a9be60e4e10616e45868d31ca3014930ba7","extensions":[],"witness_signature":"206dc384a063d35469ef2a55f5e2f9cc3223f18337c343cb7ab2220eb3f638887964c56aa726b1b26174ca538c717b543167854a5bacb9e35395b62543437534a5","transactions":[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operations":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcbe2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"],"operation_results":[[2,{"amount":3100000,"asset_id":"1.3.933"}]]}]}}
> {"id":2, "method":"call", "params":[0,"get_potential_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operations":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":[]}]]}
< {"id":2,"result":["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs"]}
> {"id":3, "method":"call", "params":[0,"get_required_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operations":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcbe2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"]},["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs"]]]}
< {"id":3,"result":["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs"]}

Copied from original issue: cryptonomex/graphene#636

[oxarbitrage]

please check:

https://github.com/bitshares/bitshares-core/blob/master/libraries/chain/include/graphene/chain/protocol/transaction.hpp#L132-L145

"The result is not always a minimal set of signatures, but any non-minimal result will still pass validation."

[abitmore]

"The result is not always a minimal set of signatures, but any non-minimal result will still pass validation."

True. But it's not hard to remove the keys which are already in the signatures.

[oxarbitrage]

on it.

[oxarbitrage]

please check:

#271

As we don't have the private key it is not possible to check if the signature of the transaction, following your sample signature:

1f1239bcbe2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee

was actually created with the available key:

BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs

But there is a loop in get_required signatures that actually was supposed to delete the key if it is repeated in both arrays.

The loop was not working propertly and the key was still showing. The pull request fix that, so following the sample.

Preliminar:

> {"id":1, "method":"call", "params":[0,"get_block",[4664932]]}
  
< {"id":1,"result":{"previous":"00472e6309ca15b5262dedd62e1e5f882fc6ee5e","timestamp":"2016-03-24T22:18:57","witness":"1.6.37","transaction_merkle_root":"68127a9be6
0e4e10616e45868d31ca3014930ba7","extensions":[],"witness_signature":"206dc384a063d35469ef2a55f5e2f9cc3223f18337c343cb7ab2220eb3f638887964c56aa726b1b26174ca538c717b5
43167854a5bacb9e35395b62543437534a5","transactions":[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operations":[[2,{"fee"
:{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcbe2e5270f5fc672
f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"],"operation_results":[[2,{"amount":3100000,"asset_id":"
1.3.933"}]]}]}}
> {"id":2, "method":"call", "params":[0,"get_potential_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operati
ons":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":[]}]]}
  
< {"id":2,"result":["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs"]}

Requesting signals with the correct key:

> {"id":3, "method":"call", "params":[0,"get_required_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operatio
ns":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcb
e2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"]},["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1
K4V9CyebNS8bBxMs"]]]}
  
< {"id":3,"result":[]}

Requesting with some other key:

> {"id":3, "method":"call", "params":[0,"get_required_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operatio
ns":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcb
e2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"]},["BTS75uzxS57o2Hey9eJHZ3zXBW9dGDjujPJzc
xGaZZqnEUjNmbMct"]]]}
  
< {"id":3,"result":["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs"]}
> 

This is i think the expected output.

[abitmore]

Requesting with some other key:
< {"id":3,"result":["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1K4V9CyebNS8bBxMs"]}

IMHO it should also return an empty set (as a supplement to OP).

[oxarbitrage]

changed pull request code, as you stated the logic was wrong.

new output:

> {"id":3, "method":"call", "params":[0,"get_required_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operatio
ns":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcb
e2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"]},["BTS5sLyeb6CzkW1MGXgDQRa1xNVcaFeN9aSD1
K4V9CyebNS8bBxMs"]]]}
< {"id":3,"result":[]}

> {"id":3, "method":"call", "params":[0,"get_required_signatures",[{"ref_block_num":11875,"ref_block_prefix":3038104073,"expiration":"2016-03-24T22:19:09","operatio
ns":[[2,{"fee":{"amount":1467,"asset_id":"1.3.0"},"fee_paying_account":"1.2.103955","order":"1.7.192217","extensions":[]}]],"extensions":[],"signatures":["1f1239bcb
e2e5270f5fc672f133b34fc25dc074474d38733fac452b8b2e8a2c03f669d5c15b267839c02aec67bfa064b482a62cb9c3f94145368edc362887288ee"]},["BTS75uzxS57o2Hey9eJHZ3zXBW9dGDjujPJzc
xGaZZqnEUjNmbMct"]]]}
< {"id":3,"result":["BTS75uzxS57o2Hey9eJHZ3zXBW9dGDjujPJzcxGaZZqnEUjNmbMct"]}

is this the correct behavior or it should return empty ? can you explain a bit further on why it should return empty ?

[abitmore]

is this the correct behavior or it should return empty ? can you explain a bit further on why it should return empty ?

Let's discuss the purpose of this API first: a client wants to know what keys are required to sign a transaction, or say, what signatures are missing to authorize a transaction. Apparently:

• if a key is not needed at all, the client doesn't need to sign with it. So the returned set should be a subset of potential keys.
• if a key is already used to sign a transaction, the client doesn't need to sign with that key again. One signature per key.
• if a key is required, but the client doesn't have it (not in available_keys), should it be returned? Current implementation is NO, perhaps we can stick to it.

So the logic should be return_key_set = required_key_set & available_keys - signature_key_set.

Here are some test cases:

• if a transaction needs [key1], signatures contain [], query with available_keys = [key1], return [key1]
• if a transaction needs [key1], signatures contain [], query with available_keys = [key2], return []
• if a transaction needs [key1], signatures contain [], query with available_keys = [key1,key2], return [key1]
• if a transaction needs [key1], signatures contain [key1], query with available_keys = [key1], return []
• if a transaction need [key1], signatures contain [key1], query with available_keys = [key2], return []
• if a transaction need [key1], signatures contain [key1], query with available_keys = [key1,key2], return []
• if a transaction needs [key1,key2], signatures contain [], query with available_keys = [key1], return [key1]
• if a transaction needs [key1,key2], signatures contain [], query with available_keys = [key2], return [key2]
• if a transaction needs [key1,key2], signatures contain [], query with available_keys = [key3], return []
• if a transaction needs [key1,key2], signatures contain [], query with available_keys = [key1,key2], return [key1,key2]
• if a transaction needs [key1,key2], signatures contain [], query with available_keys = [key2,key3], return [key2]
• if a transaction needs [key1,key2], signatures contain [], query with available_keys = [key1,key3], return [key1]
• if a transaction needs [key1,key2], signatures contain [key2], query with available_keys = [key1], return [key1]
• if a transaction needs [key1,key2], signatures contain [key2], query with available_keys = [key2], return []
• if a transaction needs [key1,key2], signatures contain [key2], query with available_keys = [key3], return []
• if a transaction needs [key1,key2], signatures contain [key2], query with available_keys = [key1,key2], return [key1]
• if a transaction needs [key1,key2], signatures contain [key2], query with available_keys = [key2,key3], return []
• if a transaction needs [key1,key2], signatures contain [key2], query with available_keys = [key1,key3], return [key1]

[abitmore]

Submitted a fix in #570. @svk31 @pmconrad would you like to check if the change will break anything?

[abitmore]

PR merged.