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

fix(core-p2p): ensure payload database exists #2851

Merged
merged 1 commit into from Jul 30, 2019
Merged

Conversation

faustbrian
Copy link
Contributor

@faustbrian faustbrian commented Jul 30, 2019

A summary of what changes this PR introduces and why they were made.

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Refactor
  • Performance
  • Tests
  • Build
  • Documentation
  • Code style update
  • Continuous Integration
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

Does this PR release a new version?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • It's submitted to the develop branch, not the master branch
  • All tests are passing
  • New/updated tests are included

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@ghost ghost added Complexity: Low labels Jul 30, 2019
@faustbrian faustbrian merged commit b190622 into master Jul 30, 2019
@ghost ghost deleted the fix/ensure-db-p2p branch July 30, 2019 04:03
@alessiodf
Copy link
Contributor

alessiodf commented Jul 30, 2019

This is a bit dangerous now. For performance reasons, and the fact this is essentially a throwaway database, it runs with no journaling. This can lead to a corrupt sqlite database in the event Core crashes, which was previously fine because the database file, if it existed, was deleted and recreated afresh on every startup. Now that won't happen so if the actual database file is corrupted or locked or whatever, it may not be able to function on restart with unexpected behaviour. (Truncating the database will fail if it's corrupted or locked.)

Also I am not sure why this was ever marked as a bug fix? What was the bug? If the database file did not exist, it would be created by new sqlite3(this.payloadDatabasePath) anyway.

@faustbrian
Copy link
Contributor Author

The bug was that on multiple machines the file was not created by better-sqlite3 which is the reason the transaction pool uses the exact same code to ensure the file actually exists because this has been an issue in the past.

TypeError Cannot open database because the directory does not exist
    at new Database (core/node_modules/better-sqlite3/lib/database.js:31:9)
    at new PayloadProcessor (core/packages/core-p2p/dist/socket-server/payload-processor.js:23:32)
    at Object.<anonymous> (core/packages/core-p2p/dist/socket-server/payload-processor.js:120:28)

There are some other minor fixes coming so will include a combination of the two in the next release as that covers everything then.

vasild added a commit that referenced this pull request Aug 2, 2019
…o-db

* ArkEcosystem/core/2.6:
  release: 2.5.0-next.10 (#2852)
  fix(core-p2p): ensure payload database exists (#2851)
  release: 2.5.14
  release: 2.5.14 (#2849)
  fix(core-p2p): peer discovery limit (#2850)
  perf(core-p2p): improve transactions endpoint (#2848)
  fix(core-api): internal server error caused by invalid orderBy field (#2847)
  refactor(core-p2p): increase network timeouts (#2828)
  fix(core-api): return data directly if cache is disabled (#2831)
  fix(core-utils): add content-type header (#2840)
  perf(core-database): lookup delegates by key (#2837)
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

Successfully merging this pull request may close these issues.

None yet

2 participants