Review plugin architecture

[ethanfrey]

I had a look at the code, and in general it makes sense. A special AppTx can set a byte (or later string?) to specify which plugin should handle it. After being verified and payment deducted by the basecoin system, the validated tx is passed to the registered plugin.

This allows us to add arbitrary tx processing with a plugin, without changing basecoin core code, just adding a plugin to the server and implementing the tx on the client.

Three issues, mostly minor.

In the current implementation, there is no way to override this in the constructor (eg. I cannot import basecoin and just init it with my special plugin). This would not be hard to change, but should be done. It is a private member and the only constructor I find is here:

func NewBasecoin(eyesCli *eyes.Client) *Basecoin {
	govMint := gov.NewGovernmint()
	state := sm.NewState(eyesCli)
	plugins := types.NewPlugins()
	plugins.RegisterPlugin(PluginTypeByteGov, PluginNameGov, govMint)
	return &Basecoin{
		eyesCli:    eyesCli,
		govMint:    govMint,
		state:      state,
		cacheState: nil,
		plugins:    plugins,
	}
}

Another issue is that the plugin cannot handle queries. Maybe this is not desired, but it may be nice to have some higher-level queries done by the plugin. But then maybe it would be unable to provide a merkle proof, and you only ever want to do queries by key. Again, this shouldn't be much work to change if it is wanted.

More questionable, is that, while the address that signed the original transaction is passed into the plugin as ctx and set in the cache
I don't see that this in any way limits the ability of the plugin to modify data from any account. Maybe I missed something. Maybe this is not needed. But it would be nice to understand the security implications of plugins. Do they have access to the entire data store and can modify anything they like? Or are they sandboxed in their own app-specific db? Or something in between?

[rigelrozanski]

Along this issue, There is no way change or charge additional fees based on information from within the plugin. I suppose this information regarding any additional fees could be passed back to basecoin after from the plugin after execution, but I don't believe there is a way to do this currently either... unless through use of the ctx? Curious if that is possible currently

[ethanfrey]

Random thoughts from chat with Rigel:

1. How to initialize money in accounts? In real world (not tests), this must be done in the genesis block, giving a small number of founders all the money? How is new money issued?

2. SetOption("base/account", value) seems to allow setting any state, is this exposed to the world? How does one trigger this? Is this synced over all nodes or just for dev/testing????

-> it seems SetOption is not really exposed in tendermint anymore? Only reference is a commented out line in proxy/appConn.go:41

3. What happens to the money for AppTx state/execution.go:109-149?
   On error, it seems that the fee just disappears. Not given to any admin account for disribution to validators or something?
   On success, it seems the entire TxInput disappears from the account, but there is no way to spend it in the plugin? Or is there?

-> Maybe we could add some special return value for the plugin, a []TxOutput. The executor can verify the total coins out = coins in. And then adds the output to the appropriate account(s). Thus the plugin could move a specified amount to a given account and leave the rest with the sender. Or any more complex logic it wants. This would have to change how executor.go works

4. It seems the entire economic decision making for money, how it is used, etc. needs to be easy to customize per deployment. Is this per-plugin? Or A higher-level, like where fees go? Fees are tied to staking and the whole PoS concept as well that needs to be configurable somehow without a hard fork of core basecoin logic.

[rigelrozanski]

Afterthoughts on point (3) from @ethanfrey. We need to resolve which parts of the transaction logic should be vested with the plugin vs basecoin executing the plugin. For instance I would imagine that for all transactions any leftover/unspent funds within the transaction should be returned to the sender's wallet (currently unsupported), this mechanism would then make sense to be built into basecoin-side of the transaction logic. On the contrary which wallet address of where fees are being moved, and how specifically fees are taken (some apps may want variable fees) I'd imagine should be built into the plugin-side of the transaction logic and then passed back to the basecoin-side to be have accounting verified and afterwards executed.
It may also be positive to still support a basic 'standard' framework of utilization of gas/fees similarly to as the current basecoin implementation (without the coins 'vanishing') but alternatively is may enough to provide standard examples and have each plugin re-implement the logic. Thoughts?

[jaekwon]

Just some thoughts before stepping out for a bit.

SetOption("base/account", value)

Currently only used upon initialization, I think in cmd/basecoin. The thought is that maybe we can set options via Tendermint's genesis.json file and set genesis options, instead of requiring a separate genesis file, OTOH there's InitChain, so maybe this should go away. So to answer your question, I think we should initialize via InitChain. But SetOption is very flexible too and allows fiddling via abci-cli, so there are pros and cons.

There is no way change or charge additional fees.
For all transactions any leftover/unspent funds within the transaction should be returned to the sender's wallet (currently unsupported)

On success, it seems the entire TxInput disappears from the account, but there is no way to spend it in the plugin? Or is there?

Context.TxInput includes all the coins minus fees, so there's a lot of flexibility here. By default the Basecoin plugin will take & destroy all the TxInput & fees, but really it should be putting those tokens to use. Or burning them, some apps will want to burn tokens.

We can return funds too, the plugin just needs to take the context's account, add any return fees, and save the account. We could make this easier as a utility function or something.

To spend it in the plugin, we'd load another user's account (say) and increment their account balance accordingly.

how specifically fees are taken (some apps may want variable fees)

The fee system is already pretty flexible and allows for variable fees.

• Looks like I forgot to include the Gas in the context.
• The Validator gets to choose whether to include the tx or not by calculating fee/gas, so it's a pretty general system currently. What we're missing is the ability for the Validator to prioritize or filter txs based on gas price, but I think we can add this in later, mostly on the Tendermint side.

Cryptoeconomically speaking, I think we should have some fee that is taken regardless of whether the tx passes or not. Then CheckTx can at least ensure that some fee will be earned, before proposing a block with this tx. Thus the distinction between fee & TxInput. (TxInput includes the fee, so Context.TxInput substracts that fee. We should probably rename CallContext.TxInput to something else that represents the concept of, "all the inputs minutes the fees".)

On error, it seems that the fee just disappears. Not given to any admin account for disribution to validators or something?

Either way, whether success or not, I think we should take the fee and eventually reimburse them to validators. So fee should also be in the context, and we should be incrementing some key/value pair in go-merkle that represents all the collected fees, and also find a way to distribute them to all the validators. This needs to happen occasionally in batch (e.g. once an hour or more) for efficiency's sake.

[jaekwon]

I don't think we want to be enforcing any token invariants (e.g. insure that all inputs = outputs) since users may want to burn tokens or even inflate them etc. Currently the design allows for maximum flexibility and enough rope to hang/hyperinflate yourself.

I do think we could benefit via some tooling to make sure that invariants are preserved, but I'm not sure that it's priority. I think our focus should first be on making it easy to use. We can focus on 100% correctness later when we have users.

[ethanfrey]

Ahh... The plugin can directly edit account balances? I thought it was sandboxed somehow from our last discussion. Anyway, if it has access to write, this should be enough. Wrt set option. I see fiddling with tmsp-cli a dev only thing. In production, quickly nodes get out of sync. Looks somehow dangerous. Dunna... Some sort of debug flag to enable it? Don't know if I want it in a production app. Brief thoughts from the train....

…

On Jan 18, 2017 8:57 PM, "Jae Kwon" ***@***.***> wrote: I don't think we want to be enforcing any token invariants (e.g. insure that all inputs = outputs) since users may want to burn tokens or even inflate them etc. Currently the design allows for maximum flexibility and enough rope to hang/hyperinflate yourself. I do think we could benefit via some tooling to make sure that invariants are preserved, but I'm not sure that it's priority. I think our focus should first be on making it easy to use. We can focus on 100% correctness later when we have users. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/tendermint/basecoin/issues/5#issuecomment-273583610>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFbSCPY4D58vDfCq_be_MUHutc4d2rrvks5rTm6mgaJpZM4Lc5__> .

[ethanfrey]

I guess this was reviewed. And this issue just remains to document the state. This should be moved into some sort of architecture docs....