ArkEcosystem / AIPs

ARK Improvement Proposals
26 stars 25 forks source link

AIP 11: Protocol Upgrade #11

Closed fix closed 6 years ago

fix commented 6 years ago

This is the comment thread on https://github.com/ArkEcosystem/AIPs/blob/master/AIPS/aip-11.md

Jarunik commented 6 years ago

Will the private transactions (with big payload) be allowed on the main chain? Would it not be better to have them on another chain keeping the main chain slim?

fix commented 6 years ago

yes this is a topic for discussion. Either we have a private transaction that replace vendor field, either we use vendorfield for that.

point is private transaction could let services register clearly their use case. So open to discussion

Jarunik commented 6 years ago

If a delegate sets C=0 he can kind of disable spam protection as the resulting fee is 0. Looks a bit too dangerous. At least high payload tx should never be completely free and should at least cost something minimal. The effect on the entire system is most likely bad enough to go for c > 0 or another formula which respects payload even though delegate sets c=0. What is your thoughts on that?

fix commented 6 years ago

However it can be wise to floor C with a minimum, but note that like bitcoin, miners could accept any tx with no fees, but in the end they don't

fix commented 6 years ago

Note also that with the current state of protocol, a valid block should embed tx with stritly > 0 fees

Moustikitos commented 6 years ago

hi,

This is a nice structure (remind me IFD tag description)

good points :

questionable point :

Moustikitos commented 6 years ago

technically, i disagree to let delegate stating for C value. Imagine, i'm a delegate and i want to make me rich... i put C=10000 and wait for only one tx... maybe 2 or 3 before i'm voted out

Jarunik commented 6 years ago

Why would anyone enter a transaction with C=10000 as fee? You will not forge anything if the transaction is below the threshold set by you.

Moustikitos commented 6 years ago

When is it planed on devnet ?

fix commented 6 years ago

@Moustikitos other delegates will choose differently anyway, so it should be ALL delegates to decide a high fee. Also it will push the price down, so people will start to unvote the "bad" delegates.

Moustikitos commented 6 years ago

When is AIP11 pushed on devnet ?

Moustikitos commented 6 years ago

So, tx will be sent hex-serialized-backed from client ? with signature, signsignature and id concatened at the end ? So node just have to read slices, run tests and extract data i presume.

Maybe the json mode could be keeped for resourceless client or node.

axenter commented 6 years ago

Why not let the sender decide what fee he wants to do? Just like with other coins like bitcoin and eth.

fix commented 6 years ago

yes in the new aip, sender will be able to adjust fees, but he should make sure it is high enough...

i was wondering if it would be better to merge type 0 and type 6 (type 0 being a special case of type6)

fix commented 6 years ago

initial implementation https://github.com/ArkEcosystem/ark-js/commit/95b591c4641ab12869890889c9e3cbbcbe457166

tpscrpt commented 6 years ago

transaction groups with more modular fee structure

faustbrian commented 6 years ago

@fix Is there a spec yet for how the verification process will look like?

Also the count of payments in a multi payment is documented to be stored as 2 bytes but @arkecosystem/crypto stores it with 4 bytes and reads it as 1 byte which results in wrong data after deserialising a transaction. Is it supposed to be 2 or 4 bytes?

sleepdefic1t commented 6 years ago

@faustbrian

I believe this is because the size of a standard Integer is dependent on the platform.

Size (bytes) Size (bits) Names Signed range Unsigned range
1 byte 8 bits Byte, octet, minimum size of char in C99( see limits.h CHAR_BIT) −128 to +127 0 to 255
2 bytes 16 bits x86 word, minimum size of short and int in C −32,768 to +32,767 0 to 65,535
4 bytes 32 bits x86 double word, minimum size of long in C, actual size of int for most modern C compilers,[1] pointer for IA-32-compatible processors −2,147,483,648 to +2,147,483,647 0 to 4,294,967,295
8 bytes 64 bits x86 quadruple word, minimum size of long long in C, actual size of long for most modern C compilers,[1] pointer for x86-64-compatible processors −9,223,372,036,854,775,808 to +9,223,372,036,854,775,807 0 to 18,446,744,073,709,551,615
unlimited/8 unlimited Bignum –2unlimited/2 to +(2unlimited/2 − 1) 0 to 2unlimited − 1

https://en.wikipedia.org/wiki/Primitive_data_type#Integer_numbers

#

Would it then be best to define it as a more specific integer type?

unsigned short 2 unsigned short int 0 to 65,535
unsigned long 4 unsigned long int 0 to 4,294,967,295

https://msdn.microsoft.com/en-us/library/s3f49ktz.aspx

Something like this may help ensure the proper size is expected on all platforms.

faustbrian commented 6 years ago

It's not an issue with that, the implementation in JavaScript is simply wrong because it is written as a different type then it is read.

Here it is written as an unsigned 32 bit https://github.com/ArkEcosystem/core/blob/master/packages/crypto/lib/models/transaction.js#L195 but it is read as a signed 8 bit https://github.com/ArkEcosystem/core/blob/master/packages/crypto/lib/models/transaction.js#L321 which results in invalid data for the asset part which contains the payments as the offset is increased by an invalid amount from that point on.

I implemented it by AIP11 spec in PHP and the multi payment works now there but the question is if 2 or 4 byte were intended as the spec states 2 but javascript uses 4 and reads as 1.

For now it is implemented by spec in PHP and all other crypto packages, except javascript, until more information is available.

fix commented 6 years ago

yes verifications rules documentation are missing, not sure if this should be part of AIP11 or other AIP number

fix commented 6 years ago

multipayment write32 is a bug should be 8

fix commented 6 years ago

well not even as for spec it should be 16...

vRobM commented 6 years ago

@alexbarnsley suggested I share this from Ark Slack: it would be interesting to abstract bulk transactions under 1 transaction ID, so that if any fail, either it all fails or you get a response as to which failed for retransmit in next round with a TTL more like a TCP of transactions right now it's very UDP, fire and forget thoughts?

roks0n commented 6 years ago

What kind of failure do you have in mind? Asking because you already get a response telling which transaction is ok and which not? Eg:

{
  "data": {
    "accept": [
      "8b1eb8c193acd5136db348aaaba83a7426f8761562945e2448e1be4f27d6bf0b"
    ],
    "excess": [],
    "invalid": [],
    "broadcast": [
      "8b1eb8c193acd5136db348aaaba83a7426f8761562945e2448e1be4f27d6bf0b"
    ]
  }
}
vRobM commented 6 years ago

Well the kinds of failures @goose is having trying to send large tx blocks. To quote him from #devnet:


vs spacing out and limiting it to the block size
If I send out 100 tx at once....all 100 get confirmed
If I send out 50 and then wait 60 seconds and send another 50........second set doesn't seem to get confirmed at all
Like the transactions vanish
Sorta at a loss as to why it's occuring```
roks0n commented 6 years ago

Would be useful to know how those unconfirmed ("vanished") ones are represented in the response - would they be under confirmed or not. I did some testing using tester-cli last week where I spammed the tx pool and didn't notice such issues.

alexbarnsley commented 6 years ago

@roks0n did you test on a testnet or devnet? We also haven't experienced the same issues

roks0n commented 6 years ago

I was running those tests on testnet at that point.

kristjank commented 6 years ago

closing this conversation. The AIP specs can be found in the AIP folder where they will be updated. For other stuff @roks0n @vRobM pleas use core/issues. Thanks