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

[2.6][T4] default ordering of locks not sufficient #3088

Closed
dated opened this issue Oct 20, 2019 · 4 comments
Closed

[2.6][T4] default ordering of locks not sufficient #3088

dated opened this issue Oct 20, 2019 · 4 comments
Assignees

Comments

@dated
Copy link
Contributor

dated commented Oct 20, 2019

The default ordering used on the locks endpoint is not meaningful enough and the returned results could theoretically, be out of order (semantically speaking).

return {
query,
entries,
defaultOrder: ["expirationValue", "asc"],
};

The expiration value would need to be properly normalised to guarantee that the returned order of locks matches the semantic order.

@ghost
Copy link

ghost commented Oct 20, 2019

Thanks for opening this issue! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@faustbrian faustbrian changed the title default ordering of locks not sufficient [2.6][T4] default ordering of locks not sufficient Oct 21, 2019
@dated
Copy link
Contributor Author

dated commented Oct 21, 2019

Normalisation would return an estimate at best, since there is no way to precisely predict the timestamp of a future block, and viceversa, there is no way to predict the blockheight at a certain point in time.

@ghost
Copy link

ghost commented Oct 25, 2019

This issue or pull request needs further investigation. Please wait for further information, thank you.

vasild added a commit that referenced this issue Oct 30, 2019
expirationValue could be either "height of the blockchain" or "seconds
since the genesis block", so sorting by it gives some misleading
impression that the returned results are sorted by expiration when they
are in fact not.

It is not possible to reliably convert from height to seconds, thus
ditch the ordering by expirationValue and order by lockId which will at
least deterministically return the results in the same order every time.

Closes #3088
spkjp pushed a commit that referenced this issue Oct 30, 2019
expirationValue could be either "height of the blockchain" or "seconds
since the genesis block", so sorting by it gives some misleading
impression that the returned results are sorted by expiration when they
are in fact not.

It is not possible to reliably convert from height to seconds, thus
ditch the ordering by expirationValue and order by lockId which will at
least deterministically return the results in the same order every time.

Closes #3088
@ghost
Copy link

ghost commented Oct 30, 2019

This issue has been closed. If you wish to re-open it please provide additional information.

kristjank pushed a commit to kristjank/bcdiploma-core that referenced this issue Mar 6, 2020
* fix(core-p2p): properly terminate bad connections (#3103)

* fix(core-database): set last height before initializing last bl… (#3109)

* refactor(crypto): overwrite arrays when merging milestones (#3108)

* refactor(core-magistrate): use type from core-interfaces (#3121)

* fix(core-api): use numerics for typeGroups in /transactions/typ… (#3112)

* fix(core-magistrate-transactions): emit correct event (#3128)

* fix(core-p2p): parallel download (#3092)

* chore(core-p2p): rename syncWithNetwork() to downloadBlocksFromHeight()

The new name better describes what the method does.

* fix(core-p2p): improve parallel block download

* Disallow gaps in the returned array of blocks.

* Cancel running jobs if some job fails.

* Do not discard downloaded blocks, lower than the ones supposed to be
  downloaded by a failed job.

* Download 10 chunks in parallel instead of 25 - it should be enough to
  saturate the connection and still not cause socketcluster timeouts.

* chore: print more details in log messages

* Make sure we dont try the same peer for different chunks at 1st

* Rename syncWithNetwork()->downloadBlocksFromHeight() also in __tests__/

* Adjust test

* Add a comment about AsyncFunction vs Promise

* Fix off-by-one error in printout

* Rewrite downloadBlocksFromHeight() tests

* Cache the downloaded chunks that were not returned to caller

* Remove redundant method PeerCommunicator.downloadBlocks()

Also improve error message printouts

* Remove unused import

* fix(core-magistrate): get connection from databaseservice (#3130)

* refactor(core-magistrate): business and bridgechain ids as numbers (#3118)

* refactor(core-database): refactor searchBusinesses & searchBridgechains (#3110)

* feat(core-api): implement businesses/bridgechains endpoint (#3119)

* feat(core-magistrate): ensure unique bridgechain name per business (#3132)

* refactor(crypto): use multiPaymentLimit from milestone if avail… (#3136)

* fix(core-blockchain): add transactions back to pool only after… (#3138)

* release: 2.6.0-next.3

* fix(core): read `paths.config` instead of env variable (#3142)

* refactor(core-magistrate-transactions): BridgechainUpdate errors (#3141)

* refactor(core-magistrate-crypto): consolidate bridgechain schem… (#3143)

* refactor(core-database): convert htlc lock vendorfield to string during bootstrap (#3145)

* fix(core-p2p): uncaught IPC timeout (#3140)

* Revert "refactor(core-magistrate-crypto): consolidate bridgecha… (#3146)

This reverts commit deef78d.

* fix(core-magistrate-crypto): improve 'name' field validation to… (#3148)

* polish(core-magistrate-crypto): remove redundant bridgechain sc… (#3150)

* refactor(core-api): fallback to core typegroup if querying by t… (#3147)

* refactor(core-magistrate): don't allow multiple business or bri… (#3137)

* fix(crypto): properly implement block size limit (#3154)

* refactor(core-magistrate-crypto): use 4 bytes instead of 8 byte… (#3156)

* fix(core-database): correctly reduce indexed bridgechain entries (#3153)

* refactor(crypto): change MaximumPaymentCountExceededError error (#3152)

* fix(core-blockchain): write accepted blocks to database before… (#3155)

* fix: dont rollback while accepted blocks have not been written to database yet

The `AcceptBlockHandler` already alters `blocksInCurrentRound` while blocks
aren't written to database until the current batch finished processing.

In the case of a fork, we'd stop while blocks have been accepted, but not
written to database yet. This led to inconsistencies and resulted
in an assert being triggered which then wrecked the state machine
effectively stopping the node.

```
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
  assert_1.default(this.blocksInCurrentRound.pop().data.id === block.data.id)
    at DatabaseService.revertBlock (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-database/dist/database-service.js:312:25)
    at async revertLastBlock (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:236:13)
    at async __removeBlocks (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:260:13)
    at async __removeBlocks (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:261:13)
    at async Blockchain.removeBlocks (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/blockchain.js:270:9)
    at async startForkRecovery (/home/ark/.config/yarn/global/node_modules/@arkecosystem/core-blockchain/dist/state-machine.js:204:9)
```

This PR ensures that a fork only starts after all accepted blocks are
in the database. Tested manually, before I was able to reproduce it every
time, now I can no longer reproduce it.

* refactor(crypto): update multipayment limits (#3157)

* fix(core-p2p): timeout promise race (#3158)

* feat(core-api): add additional fields to businesses search sche… (#3160)

* chore(core-p2p): improve log message (#3161)

* fix(core-p2p): call next() so that the request can proceed (#3163)

* refactor(core-magistrate-crypto): use URI schema for website an… (#3162)

* refactor(core-p2p): pluralize logs (#3168)

* refactor(core-api): more restrictive wallet id schema (#3166)

* feat(core-container): log-process-errors (#3174)

* feat(core-p2p): implement throttling on outgoing p2p communication (#3170)

Use the rate limiter in reverse to stop ourselves from sending too
many requests to a given peer that could possibly trigger their
protection and result in banning us.

We use the default configuration for rate limits. So this will work as
long as the peer is using the same software and has not changed his
config or has made it more liberal. If the peer has made his limits
more restricted, then we could still trigger their protection.

A more robust solution would be to negotiate the rate limits at peer
handshake and then obey different limits for each peer.

* fix(deps): update dependency newrelic to v6 (#3177)

* fix(core-transactions): add `delegate.producedBlocks` attribute (#3178)

* feat(core-api): add additional fields to bridgechains search sc… (#3165)

* fix(core-p2p): return error when app is not ready (#3171)

* release: 2.6.0-next.4

* chore(crypto): add 2.6.0-next.4 as aip11 minimum version

* chore(core-database): remove misleading order by expirationValue (#3181)

expirationValue could be either "height of the blockchain" or "seconds
since the genesis block", so sorting by it gives some misleading
impression that the returned results are sorted by expiration when they
are in fact not.

It is not possible to reliably convert from height to seconds, thus
ditch the ordering by expirationValue and order by lockId which will at
least deterministically return the results in the same order every time.

Closes ArkEcosystem/core#3088

* refactor(core-transactions): rename handler parameter, check ag… (#3183)

* fix(core-transactions): reindex wallet when applying delegate resignation (#3185)

* fix(crypto): validate address of multi payment recipients (#3187)

* test(e2e): update framework (#3190)

* fix(core-api) include typeGroup in `/transactions/fees` and `/node/fees` endpoints (#3193)

* refactor(core-magistrate-transactions): require static fee (#3194)

* chore(deps): update dependency depcheck to ^0.9.0 (#3192)

* fix(core-state): remove delegate.rank for resigned delegates (#3202)

Fixes ArkEcosystem/core#3186

* fix(core): prevent snapshot commands from running if core is running (#3196)

* fix(core-transactions): allow unvoting a resigned delegate (#3201)

* feat(core-api): include business asset in wallet transformer (#3205)

* ci: add dependency caching (#3206)

* fix(core-p2p): handle unwanted control frames

* fix(core-p2p): stricter WS/SC events/messages handling

* fix(core-magistrate-transactions): check for an exception before checking for invalid fees (#3215)

* chore(crypto): add static fee exceptions (#3214)

* chore(deps): update dependency @types/depcheck to ^0.9.0 (#3211)



Co-authored-by: Renovate Bot <bot@renovateapp.com>

* refactor(core-transactions): unique ipfs hashes (#3204)

* fix(core-transactions): add ipfs exception handling (#3217)

* test(core-p2p): fix mock common block (#3220)

common block response only has a few properties

* feat(core-api): add `isExpired` property to locks response (#3221)

* fix(core-transactions): htlc bootstrap (#3213)

* feat(core-magistrate-transactions): ensure unique genesisHash per bridgechain  (#3199)

* chore: update to TypeScript 3.7 (#3230)

* feat(core-api): filter locks by expiration status (#3227)

* feat(crypto): implement Address.fromWIF method (#3228)

* refactor(core-api): validate expiration type based on enum (#3226)

* refactor(crypto): move verifySignatures into Transactions.Verifier (#3231)

* fix(core-transactions): add additional bridgechain registration exception handling (#3222)

* refactor(crypro): make deserializers static (#3234)

* fix(deps): update dependency chalk to v3 (#3235)

* fix(core-magistrate-transactions): case insensitive bridgechain comparison (#3233)

* refactor(crypto): adjust generic name schema (#3229)

* fix(core-api): include query in wallets/{id}/locks cache (#3240)

* refactor(core-api): add schema for orderBy query param (#3243)

* fix: return early only if there are rows (#3244)

* refactor(crypto): use transactionId ref in lockTransactionId schema definition (#3246)

* fix(crypto): adjust genericName regex and add tests (#3247)

* refactor(core-magistrate-transactions): more verbose static fee mismatch error (#3248)

* fix(core-transactions): do not attempt to convert vendorfield (#3252)

* feat(core): add command to clear transaction pool (#3250)

* fix(crypto): replace ipfs exception (#3253)

* fix(core-transaction-pool): wallet-manager fallback to database wallet manager findByIndex() when no "local" match (#3256)

* fix: wallet-manager findByIndex
try find index with db wallet manager if no match "locally"

* test: wallet-manager findByIndex

* style: replace native forEach with for-of (#3260)

* fix(deps): update dependency envfile to v4 (#3266)



Co-authored-by: Renovate Bot <bot@renovateapp.com>

* fix(core-magistrate): use genesisHash for bridgechainId (#3258)

* fix(core-forger): don't swallow BIP38 errors (#3271)

* fix(core): remove password flag constraint for core:forger command (#3270)

* fix(core-magistrate): use wallet publicKey for business id index (#3268)

* refactor(crypto): Set transactionBaseSchema fee minimum to 0 (#3275)

* chore(deps): update dependency @types/node-forge to ^0.9.0 (#3274)

* test(core-forger): fix bip38 failure expectations

* refactor(crypto): set minimum fee on transaction types (#3278)

* chore: handle Debian/Ubuntu derivate NodeJS install (#3272)

* chore(deps): update node.js to v12.13.1 (#3279)

* test(core-magistrate): business / bridgechain e2e workflow (#3280)

* chore: better handling Debian/Ubuntu derivatives NodeJS install (#3283)

* feat(core-magistrate-crypto): add ports to bridgechain registration/update (#3255)

* feat: Make roundInfo optional in databaseService.getActiveDelegates() (#3276)

* release: 2.6.0-next.5

* chore(crypto): bump minver to next.5

* release: 2.6.0-next.6

* chore: update yarn.lock

* fix(core-api): search by genesisHash in show method (#3293)

* feat(core-api): allow searching businesses and bridgechains by isResigned (#3292)

* refactor(crypto): fix genesis and exception transactions cache (#3296)

* Proper memoization

* Attempting to reduce code complexity

* Move memoized functions up

* Using whole network configuration for memoization

* fix(core-transactions): update sender's wallet after validation (#3291)

* fix(core-transactions): add missing delegate attributes to walletAttributes (#3304)

* feat(core-p2p): increase maximum token symbol length (#3311)

* fix(core): fix "file path exists" error (#3321)

* chore: add tmp-shm and tmp-wal to .gitignore (#3317)

* refactor(core-api): accept address, publicKey, delegate name as business id (#3310)

* fix(core-database): handle forgedTotal and voteBalance in orderBy query param (#3320)

* chore: document canEnterTransactionPool() (#3323)

Explain that if canEnterTransactionPool() returns false it would have
called processor.pushError().

In addition:

Simplify some code that did "if (A) { return false } return true"
to "return !A".

Remove redundant processor.pushError() from
DelegateResignationTransactionHandler() because
typeFromSenderAlreadyInPool() would have already pushed a very similar
error message.

Closes ArkEcosystem/core#3322

* fix(core-api): businesses/bridgechains search by name (#3313)

* test(e2e): simplify e2e init & network launch (#3333)

* fix(crypto): throw error instead of omitting long vendorField values (#3338)

* fix(core): network generate command (#3334)

* fix(core-database): don't assume blocksInCurrentRound is defined (#3341)

When resetting this.blocksInCurrentRound to an empty array assign [] to
it which also works if it is undefined.

Fixes ArkEcosystem/core#3329

* refactor(core-api): add shared username schema (#3344)

* fix(deps): update dependency semver to v7 (#3346)



Co-authored-by: Renovate Bot <bot@renovateapp.com>

* chore(deps): update node.js to v12.14.0 (#3351)

* chore(deps): update dependency @sindresorhus/tsconfig to ^0.7.0 (#3348)



Co-authored-by: Renovate Bot <bot@renovateapp.com>

* fix(core-state): use parent findByPublicKey method for temp-wallet-manager (#3336)

* refactor: make the canEnterTransactionPool() interface more robust (#3342)

Make it impossible for an implementation of canEnterTransactionPool() to
return false (cannot enter) and forget to push an error to the list of
errors. Change is so that it returns an error object, with null return
value meaning "no error, can enter".

Related to ArkEcosystem/core#3322

* fix(core magistrate): business/bridgechain update handlers - revertForSender (#3350)

* core-p2p: relax postBlock rate limit (#3352)

This allows two blocks to be sent/received shortly one after another, in
case we have late blocks.

Will maybe fix ArkEcosystem/core#3349

* refactor(crypto): throw error if multi payment has less than two payments (#3345)

* refactor: align maxPayload / maxTransactionBytes values (#3287)

* feat(core-api): stricter resource-based orderBy schema (#3319)

* release: 2.6.0-next.7 (#3356)

* refactor(core-p2p): discover peers when checking network health (#3358)

* refactor: enable htlc types via dedicated milestone (#3353)

* fix(core-p2p): master merge fixes for core-p2p worker (#3359)

* refactor: call `checkNetworkHealth` more deterministically (#3360)

* fix(core-database): allow legacy transaction in getBlocksForRound (#3372)

* fix: allow legacy transaction in getBlocksForRound
use deserializeTransactionsUnchecked option to disable checks
(as we get the block and txs from our own db)

* chore(deps): update node.js to v12.14.1 (#3376)

* refactor(core-api): wallet transform (#3312)

* feat(core-magistrate): add bridgechain asset repository (bridgechain registration and update) (#3375)

Add bridgechain "asset repository" in bridgechain registration (optional field) + allow update with bridgechain update transaction.

Allow update of bridgechain main repository with bridgechain update transaction.

* refactor(core-api): do not include /api prefix in pagination (#3381)

* feat(core-api): retrieve business by username, address or public key (#3379)

* fix(core-p2p): block payloads with additional properties

* fix(core-p2p): disconnect if event does not have a defined handler

* fix(core-p2p): block invalid opcode packets

* fix(core-p2p): disconnect existing connections from the same ip

* perf(core-p2p): use shared rate limiter for defined endpoints only

* refactor(core-blockchain): call `checkNetworkHealth` less deterministically

* fix(core-p2p): stop processing blocks containing too many transactions

* core-transactions: fix HTLC unlock secret format

Previously the HTLC unlock secret (preimage) would be required to be 32
printable ASCII characters.

Fix this so that it can be any 32 bytes. To accommodate this we print it
in hex (64 chars) in the HTLC claim transaction JSON.

* refactor: custom validation for postTransactions
as we cant validate with ajv in worker for transactions
implemented a "light validation" to quickly discard wrong data
this will need to be reworked with p2p for v3

* refactor: ban if remote opens multiple sockets

* test: fix integration tests

* refactor: disable peer check for postBlock/postTransactions

* fix(crypto): relax network interface check for seednode ips (#3391)

* Revert "refactor: ban if remote opens multiple sockets"

This reverts commit eb021e9.

* test: fix p2p socket test

* test(core-magistrate): business tests (#3398)

* chore(deps): update dependency jest to v25 (#3414)

Co-authored-by: WhiteSource Renovate <renovatebot@gmail.com>

* refactor(core-p2p): remove unnecessary termination (#3415)

* feat(core-magistrate): implement dynamic fees (#3416)

* release: 2.6.0-next.8 (#3418)

* release: 2.6.0-next.9 (#3419)

* chore(deps): update dependency ts-jest to v25 (#3420)

* syntax: isSupportedTansactionVersion > isSupportedTransactionVersion (#3421)

* chore(deps): update dependency cross-env to v7 (#3422)

* chore: bcdiploma testnet config

* fix(core-blockchain): pass IBlockData to processBlocks instead of IBlock (#3426)

* chore: devnet config for bcdiploma

* perf(core-database-postgres): use type index when querying multipayments (#3430)

* chore (config): fixed milestones

* chore: mainnet network configuration

* chore: testnet explorer fix link

* chore: adding devnet peers to the list

* chore: update peers.json for devnet

* chore: lower minimal network reach for devnet

* chore: adding certificate manager to right place in config

* chore: adding required submodules

* Update .gitignore

* docs: 2.6.0 changelog (#3435)

* fix(core-tester-cli): set network height on configManager (#3432)

* chore: add plugins to separate process registrations.

* chore: adjusting epoch date

* chore: removing magistrate from plugins loading

* refactor(core-api): re-enable username and 2nd public key as top-level api attributes (#3437)

* refactor(core-p2p): timeouts for peer socket messages (#3439)

* fix(core-transactions): apply genesis wallet exception to lastForgedBlocks (#3440)

* release: 2.6.0-next.10 (#3441)

* release: 2.6.0 (#3454)

* chore(core-p2p): merge patch from master (#3456)

* chore: update aip11 milestone + update p2p minver defaults (#3460)

* test(core-p2p): fix minver unit test (#3462)

* chore(exchange-json-rpc): use @arkecosystem/exchange-json-rpc@2.0.0 (#3458)

* release: 2.6.1

* feat(core-api): filter peers by version range (#3465)

* feat(core): add flag to skip export of rolled back transactions (#3459)

* refactor(core-transactions): use findByPublicKey to set both publickey and address on the multisig wallet (#3498)

* fix(crypto): multisig legacy allow signatures property (#3489)

* fix(core-database): check for missed blocks before applying round (#3507)

* chore: remove pm2 from docker (#3505)

* refactor(crypto): remove long dependency (#3502)

* fix(core): make app.js optional as initially intended (#3510)

* release: 2.6.9 (#3515)

* release: 2.6.0-next.11

* fix(core-p2p): disable permessage-deflate (#3518)

* release: 2.6.10

* refactor(core-magistrate): allow multiple ports in bridgechain schema (#3504)

* refactor(core-magistrate): allow to resign business only when bridgechains are resigned (#3524)

* refactor(core-magistrate): make bridgechain genesis hash only unique per wallet (#3523)

* fix(core-p2p): only accept valid http path (SC http server) (#3537)

* release: 2.6.11 (#3538)

* release: 2.6.0-next.12

* chore: update readme links (#3540)

* chore(magistrate): add exceptions for business resignation (#3551)

* release: 2.6.0-next.13

* release: 2.6.12-next.13

* release: 2.6.12-next.15

* refactor(core-transaction-pool): no default addonBytes for magistrate transactions (#3560)

* perf(core): use jemalloc as the memory allocator (#3541)

* fix(core-blockchain): set height 1 on config manager for processing genesis block (blockchain replay) (#3561)

* fix(core): handle multiple installations of jemalloc (#3562)

* fix(core): jemalloc compatibility for ubuntu 16.04 (#3567)

* fix(core-transaction-pool): always call applyToRecipient (#3570)

* release: 2.6.21

* chore(deps): update xstate to v4.8.0 (#3575)

* refactor: use application events from core-event-emitter (#3574)

* fix(core-database-postgres): add missing transactions.type_group index (#3573)

* ci: setup auto publishing for master releases (#3576)

* release: 2.6.24

* ci: ensure we are in the packages directory before publishing

Co-authored-by: alessiodf <35549818+alessiodf@users.noreply.github.com>
Co-authored-by: supaiku <1311798+supaiku0@users.noreply.github.com>
Co-authored-by: Edgar Goetzendorff <hello@dated.fun>
Co-authored-by: Vasil Dimov <vd@freebsd.org>
Co-authored-by: Brian Faust <hello@basecode.sh>
Co-authored-by: Lemii <peter.spaargaren@gmail.com>
Co-authored-by: Brian Faust <faustbrian@users.noreply.github.com>
Co-authored-by: air1one <36802613+air1one@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: supaiku <supaiku@protonmail.com>
Co-authored-by: Air1 <erwann@ark.io>
Co-authored-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: Dean van Dugteren <31391056+deanpress@users.noreply.github.com>
Co-authored-by: Adrian Kerchev <akerchev@gmail.com>
Co-authored-by: Dmitry Tyshchenko <rainydio@gmail.com>
Co-authored-by: Guillaume NICOLAS <guillaume.nicolas@spacelephant.org>
Co-authored-by: KovacZan <39158639+KovacZan@users.noreply.github.com>
Co-authored-by: WhiteSource Renovate <renovatebot@gmail.com>
Co-authored-by: ItsANameToo <35610748+ItsANameToo@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants