CityOfZion / neo-python

Python Node and SDK for the NEO 2.x blockchain. For NEO 3.x go to our successor project neo-mamba
https://neo-python.readthedocs.io/en/latest/
MIT License
313 stars 189 forks source link

Simple Policy Plugin #960

Closed saltyskip closed 5 years ago

saltyskip commented 5 years ago

Current behavior

Python nodes currently do not support the Simple Policy plugin on the network. Transactions that do not meet the requirements will be accepted in the node, but stuck in the mempool.

Expected behavior

Implementation of Simple Policy plugin for neo python

How to reproduce

Submit large transaction via RPC to python node. If over 1024 bytes and no fee is attached, the transaction should fail. In NEO python it will be accepted into the mempool

Your environment

RPC

ixje commented 5 years ago

Thanks for reporting.

Is there already an unambiguous description of the feature? It has been discussed last week but there were different interpretations of its working based on the news.

jseagrave21 commented 5 years ago

@saltyskip @ixje I can confirm this behavior. When we implemented the SimplePolicyPlugin in https://github.com/CityOfZion/neo-python/pull/869, we only implemented for relaying transactions, not if a rawtransaction was sent to the node and then verified. I have since implemented similar logic in the Verify function and tested.

hal0x2328 commented 5 years ago

The SimplePolicy implementation in #964 works, but the transaction fee is still calculated incorrectly. I believe this is because the transaction is built is using the network fee as calculated from test_invoke that is based on the unsigned transaction size, but the signed transaction size is larger, leading to a failure during Transaction.Verify.

I added an extra debugging line locally to test_invoke to print the wallet_tx.Size() it was basing the fee off of, and then tried to deploy a contract:

Calculating required fee for transaction with size: 11299

-------------------------------------------------------------------------------------------------------------------------------------
Test deploy invoke successful
Total operations executed: 11
Results:
[<neo.Core.State.ContractState.ContractState object at 0x1147a12b0>]
Deploy Invoke TX GAS cost: 490.0
Deploy Invoke TX Fee: 0.10275
-------------------------------------------------------------------------------------------------------------------------------------

Enter your password to deploy this contract on the blockchain
[password]> **********************                                                                                                               
[D 190717 09:06:09 Transaction:596] Verifying transaction: b'abf44004b7176d16f62ec404c29384f71011499a38d8fe6609005e1e5d14d194' (size: 11427 bytes)
[D 190717 09:06:09 Transaction:608] The tx size (11427) exceeds the max free tx size (1024).
    A network fee of 0.10403 GAS is required.
[E 190717 09:06:09 mempool:28] Verifying tx result... failed
Could not relay tx abf44004b7176d16f62ec404c29384f71011499a38d8fe6609005e1e5d14d194 

You can see test_invoke calculated the transaction size as 11299 but after signing, Verify calculated it as 11427, so the attached fee was insufficient to continue.

jseagrave21 commented 5 years ago

@hal0x2328 thank you for looking into this. So as I understand it, #964 is still valid but I need to move the logic for the SimplePolicy in #869 to after the tx is signed. This makes a lot of sense.

ixje commented 5 years ago

@jseagrave21 do you want to look into this? Perhaps this can help https://github.com/neo-project/neo-plugins/blob/master/SimplePolicy.UnitTests/UT_SimplePolicy.cs

jseagrave21 commented 5 years ago

@ixje yes, I will look into it. I will proceed as mentioned above. It shouldn't take me too long.