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

misc(core-blockchain): log the reason for discarding a block #2903

Merged
merged 7 commits into from
Sep 6, 2019

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Sep 3, 2019

When processing blocks, if we find that some block is not chained then
we abandon the rest of the blocks. In this case, print a detailed
message explaining that a block is not chained and why.

Without this printout sync failures could be very obscure and difficult
to analyze.

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)

When processing blocks, if we find that some block is not chained then
we abandon the rest of the blocks. In this case, print a detailed
message explaining that a block is not chained and why.

Without this printout sync failures could be very obscure and difficult
to analyze.
@ghost
Copy link

ghost commented Sep 3, 2019

Your pull request doesn't follow our contribution guidelines. Please review and correct it.

@ghost ghost added the Complexity: Low label Sep 3, 2019
@codecov-io
Copy link

codecov-io commented Sep 3, 2019

Codecov Report

Merging #2903 into develop will decrease coverage by 0.41%.
The diff coverage is 53.84%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2903      +/-   ##
===========================================
- Coverage    68.17%   67.76%   -0.42%     
===========================================
  Files          406      424      +18     
  Lines         9767     9932     +165     
  Branches       507      512       +5     
===========================================
+ Hits          6659     6730      +71     
- Misses        3067     3158      +91     
- Partials        41       44       +3
Impacted Files Coverage Δ
packages/core-blockchain/src/blockchain.ts 63.47% <100%> (+0.22%) ⬆️
packages/core-utils/src/is-block-chained.ts 60% <33.33%> (-40%) ⬇️
...s/core-interfaces/src/core-database/event-types.ts 0% <0%> (ø)
packages/core-transaction-pool/src/plugin.ts 100% <0%> (ø)
...aces/src/core-database/search/search-parameters.ts 0% <0%> (ø)
packages/core-blockchain/src/plugin.ts 0% <0%> (ø)
packages/core-wallet-api/src/plugin.ts 0% <0%> (ø)
packages/core-logger-winston/src/plugin.ts 75% <0%> (ø)
packages/core-logger-pino/src/plugin.ts 75% <0%> (ø)
packages/core-p2p/src/plugin.ts 70.83% <0%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97be5df...be74f45. Read the comment docs.

packages/core-utils/src/is-block-chained.ts Outdated Show resolved Hide resolved
@ghost ghost added the Status: Needs Changes label Sep 3, 2019
@ghost
Copy link

ghost commented Sep 3, 2019

Your pull request needs some changes. Please wait for a comment from one of our developers for more information.

@spkjp spkjp changed the title Print the reason for discarding a block misc(core-blockchain): log the reason for discarding a block Sep 3, 2019
vasild and others added 6 commits September 4, 2019 10:25
…k-chained-verbose

* ArkEcosystem/core/develop:
  test: mock missing wsServer
  release: 2.5.24 (#2908)
  fix(core-p2p): terminate blocked client connections
  fix(core-p2p): drop connections with malformed messages
  fix(core-api): return block timestamp for v2 transactions (#2906)
  fix(core-blockchain): only shift milestoneHeights[] if at that height (#2904)
  fix(crypto): use `anyOf` for transactions schema (#2894)
  fix(core-webhooks): cast params in condition checks (#2887)
  feat(core-p2p): use compression on the p2p level (#2886)
This would make us try to download the block at the same height again
from a different peer.

Also, check if the block being tested for isBlockChained() is in the
list of exceptions and if so, assume the check has passed.
…k-chained-verbose

* ArkEcosystem/core/develop:
  refactor(core-p2p): share rate limiter between workers (#2912)
@spkjp spkjp self-requested a review September 6, 2019 13:46
@spkjp spkjp merged commit 4d34a1d into develop Sep 6, 2019
@ghost ghost deleted the is-block-chained-verbose branch September 6, 2019 23:50
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.

3 participants