Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PoW Spam protection (TX V2) #440

Closed
wants to merge 1 commit into from
Closed

Conversation

shargon
Copy link
Member

@shargon shargon commented Nov 1, 2018

@shargon shargon requested review from erikzhang and belane November 1, 2018 12:26
@erikzhang
Copy link
Member

I doubt if PoW is really useful. Because controlling thousands of devices for PoW attacks is an easy task.

@shargon
Copy link
Member Author

shargon commented Nov 1, 2018

I agree with you, but reduce the attack surface is good for all

@erikzhang
Copy link
Member

In my opinion, although this reduces a little attack surface, it will have a relatively large impact on the average user.

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2018

@shargon, sorry to say that, but I am strongly against PoW strategies here in the Neo Ecosystem.
The goal is much different nowadays. We have been discussing several other strategies.

I still believe in several other ideas that takes use of current potential of our Computational Intelligence tools.

10 years ago (yesterday), they did not have the tools we have nowadays. We have tons of strategies for filtering spam in a transparent manner.
It is just a matter of focus on our efforts.

@ThomasLobker
Copy link

I still think it's better to use this for sorting/prioritizing than to use this as a threshold. When we apply this strategy to prioritize transactions, then we don't need to waste CPU cycles when the network is running idle. When the network is congested, the user can choose to wait or to increase the difficulty.

@shargon
Copy link
Member Author

shargon commented Nov 1, 2018

I want to implement all kind of solution for filter and drop spam.
This type of attacks is becoming more frequent, and this should be a priority.
We can implement the different solutions and then decide. But we need solutions, i think that all of us are agree with that.

I do not care if it is this, or another solution, I just want to solve the problem.

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2018

I understand you, @shargon, but, personally, I believe that this will not solve the problem and also has the potential to weaken the Ecosystem.

Something more plausible to me would goes in this direction:

  • Mem_pool with free transactions (already implemented);
  • Mem_pool with fee's transactions (already implemented);
  • Mem_pool with staked transactions (To be done);
  • Mem_pool with PoW transactions (To be done - if the community agrees);

Each of them would have predefined boundaries of amount of transactions to be inserted in the block.

There is also a problem nowadays, the 20 free txs are not respected if there are more than 500 fee based tx (https://github.com/neo-project/neo-plugins/blob/ada292dec3c93cd478a101937f99af84a36654b8/SimplePolicy/SimplePolicyPlugin.cs#L35). My suggestion is a limit of transactions (such as 500) but also limits for each class of transactions, for example (among these 500): 20 (free), 380 (fee), 50 (staked), 50 (Pow).

We would need a system that respect the limits of each class of Transactions.

With a proposal like this we could let space for different solutions and proposals to keep being implemented for each class of transactions.
Then, there would be opportunities for different developers and cryptographers to idealize their ideas.

@shargon shargon added the Discussion Initial issue state - proposed but not yet accepted label Nov 1, 2018
@canesin
Copy link
Contributor

canesin commented Nov 1, 2018

@vncoelho we need to stop negating basic economic principles. Things that consume resources should cost something to the beneficiary. PoW is a indirect fee that anyone can pay without going through the problem of acquiring some token, it lowers a lot the barrier of entry and can be easily embedded in a webapp to interact with NEO blockchain if needed, so if we keep our current sorting like @ThomasLobker suggest OrderByFees(OrderbyPoW(Mem_pool)) it should be effective coupled with @belane drop of transactions.

@shargon is there a easy way a wallet can see what is the current difficulty to send a transaction ? Because we can add to wallets so that they can display a message of the likes "there is currently congestion on the network, click continue to attach a proof-of-work to speedup the processing of this transaction".

Sorry for non-technical comment regarding the PR on this thread

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2018

@canesin, I agree with you and I like this sentence ("we need to stop negating basic economic principles"), but, for example, on the same line of reasoning, staking means a cost to the beneficiary.
On the same way, I understand this PoW strategy.

While I see this easy minor Proof-Of-Work applicable for normal users, we should consider that Exchanges are big participants of the Ecosystem in normal days operation.
How can we expect that Exchanges are going to waste their resources into this? Or are you also planning a third-party service for attaching the PoW in a middle-layer?

We should move in a way that previous concepts and future concepts can work in parallel.
That is why the fixed limits for each mempool class is important if these concepts discussed here becomes reality.

@igormcoelho
Copy link
Contributor

igormcoelho commented Nov 1, 2018

@canesin I agree, free tx must be paid somehow: rich pay with stake and poor pay with work (crazy blockchain world will look like crazy real world...).

@vncoelho also agree with you, we should save space for 480 paid tx and 20 free, ordered by stake or work (priority for stake, in my opinion, to not interfere with exchanges).

@shargon congratulations for the initiative, but your PR is too complex, lets simplify it a lot to get it done quickly. With few lines we can get PoW and PoS working simultaneously and smoothly. May I change yours or create a new one?

@vncoelho
Copy link
Member

vncoelho commented Nov 1, 2018

I also suggest a title like: "Create new MemPool for PoW and PoS transactions".

@igormcoelho, I see, brother, in this sense, you prefer keeping the same strategy of today and just order by stake and work instead of isolated mempool(dictionary) and "competitions" for each class?

The problem is to decide who gonna be the first in this order, PoS or PoW or Higher Fees? It is a multi-criteria decision making problem.
In this sense, I prefer to dedicate a specific slot for each of them and just order by their whole and later ThenBy(Fee).

@igormcoelho
Copy link
Contributor

@vncoelho I think stake can have priority over PoW on free tx. It's harder to have higher stakes than doing useless PoW.

@shargon I think this PR here (#443) will save a lot of work in yours here... please take a look whenever you can ;)

@bpetridis
Copy link

bpetridis commented Nov 2, 2018

Forgive my ignorance but would the hybrid PoS/PoW model proposed have prevented the NNS airdrop congestion incident that occurred yesterday? Have a feeling their holdings may have bypassed that measure. Still think there is a lot of work for what could be fairly easily addressed by a mandatory 0.001 GAS fee on everything. Just my personal opinion. Easier said than done.

@igormcoelho
Copy link
Contributor

igormcoelho commented Nov 2, 2018

@bpetridis I think PoS is necessary to save money from dex, the rest can easily pay any fixed value. Stil, PoW is important to regular users, imagine you have some assets, no Neo, and no Gas. How do you tranfer? you cannot claim, so you need to buy gas somewhere, just to move your funds... PoW is very useful on these scenarios.
Without free alternatives, a global fee may harm many business..

@bpetridis
Copy link

bpetridis commented Nov 2, 2018

@igormcoelho thanks for the clarification, yes this makes sense. But how would we selectively paramaterise the code to differentiate between a DEX being the originator as opposed to a very large wallet such as NNS the other day? Guess that edge case may not be handled by this individual mod which I really agree with BTW, enforcing some level of work to private wallets to prevent malicious or accidental spamming makes complete sense to me as one of the collection of measures that have and are going in like #414 and some of the discussion items in #428 . God speed 😁😁

@@ -15,6 +15,7 @@ internal class Settings
public IReadOnlyDictionary<TransactionType, Fixed8> SystemFee { get; private set; }
public Fixed8 LowPriorityThreshold { get; private set; }
public uint SecondsPerBlock { get; private set; }
public uint TransactionDifficult { get; private set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we don't need to establish a difficulty, we just compare user transactions... the ones starting with more zeroes get priority.

{
// Check PoW spam protection for drop TX

if (tx.Difficult > Settings.Default.TransactionDifficult)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we don't need to establish a difficulty, we just compare user transactions... the ones starting with more zeroes get priority.

if (Version >= 2)
{
writer.Write(CreationHeight);
writer.Write(Nonce);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User can select any CreationHeight? Even if it's very old? It could pre-compute all tx before, to prepare mass attacks. I don't think Nonce is necessary, if Remark fields are already being used to provide transaction randomization.

do
{
_hash = null;
Nonce = (uint)rand.Next();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do this with a Remark field?

@@ -97,6 +100,8 @@ public virtual Fixed8 NetworkFee

public virtual Fixed8 SystemFee => Settings.Default.SystemFee.TryGetValue(Type, out Fixed8 fee) ? fee : Fixed8.Zero;

public uint Difficult => (uint)BitConverter.ToInt32(Hash.ToArray().Take(4).Reverse().ToArray(), 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice Shargon! But it's assuming a standard difficulty of 4 zeroes... perhaps we just leave users compete for difficulty? More zeroes, more chance to get free tx.

@@ -8,8 +8,6 @@ namespace Neo.Network.P2P.Payloads
{
public class MinerTransaction : Transaction
{
public uint Nonce;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this useful here to provide randomization of Miners tx?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now is on all TX, so is reused

@@ -34,7 +34,8 @@ public ConsensusService(NeoSystem system, Wallet wallet)

private bool AddTransaction(Transaction tx, bool verify)
{
if (context.Snapshot.ContainsTransaction(tx.Hash) ||
if (tx.Version < 2 || // Don't accept TX without PoW spam protection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can build this without needing a new tx version... perhaps just using Remark fields as Nonce, and leaving difficulty be the number of zeroes in tx hash itself.

@shargon shargon closed this Jan 18, 2019
@shargon shargon deleted the tx-v2 branch January 18, 2019 12:10
@vncoelho
Copy link
Member

As explained by @igormcoelho, Prof-of-Work is currently working here, @shargon:

lastTransaction.Hash.Should().BeGreaterThan(tx.Hash);

We can slightly improve that test to be a little bit more sure about that.

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants