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 incorrect start height for block request #5875

Merged
merged 2 commits into from Dec 6, 2021

Conversation

chimp1984
Copy link
Contributor

We added 1 as with the lite monitor mode we persist the most recent block,
thus we request with the start height for the next block.
But that cause a problem at a DAO full mode which has lite monitor mode set
as then the block parsing would not be triggered.

We refactor it so that we take the chainHeight from the dao state
directly and add 1 at the requests.
We add a check if we are at chain tip, and if so we skip requests
and call the onParseBlockChainComplete directly.

How to test it?
Use a DAO full node and set lite mode for monitoring in setting.
Create a new block. Restart. Now there is no BSQ and no monitoring data as block parsing did not get triggered.
With the PR this does not happen.

We added 1 as with the lite monitor mode we persist the most recent block,
thus we request with the start height for the next block.
But that cause a problem at a DAO full mode which has lite monitor mode set
as then the block parsing would not be triggered.

We refactor it so that we take the chainHeight from the dao state
directly and add 1 at the requests.
We add a check if we are at chain tip, and if so we skip requests
and call the onParseBlockChainComplete directly.
@ripcurlx ripcurlx added this to the v1.8.0 milestone Nov 30, 2021
@ripcurlx
Copy link
Contributor

How to test it?
Use a DAO full node and set lite mode for monitoring in setting.
Create a new block. Restart. Now there is no BSQ and no monitoring data as block parsing did not get triggered.
With the PR this does not happen.

I tried to re-produce this issue on Regtest, but without success. All my nodes on Regtest are full nodes without monitoring and I didn't recognized the failed behavior before or when following the steps provide above. Is this only reproducible on Mainnet?

@ripcurlx
Copy link
Contributor

This change causes fresh Mainnet light nodes to finalize the block parsing until the next block is received. So they seem to be stuck on DAO Syncronization until a new block is received.

Bildschirmfoto 2021-11-30 um 11 55 18

@ghubstan
Copy link
Member

How to test it?
Use a DAO full node and set lite mode for monitoring in setting.
Create a new block. Restart. Now there is no BSQ and no monitoring data as block parsing did not get triggered.
With the PR this does not happen.

I tried to re-produce this issue on Regtest, but without success. All my nodes on Regtest are full nodes without monitoring and I didn't recognized the failed behavior before or when following the steps provide above. Is this only reproducible on Mainnet?

Me too, I could not reproduce the problem in regtest mode.

@chimp1984
Copy link
Contributor Author

Have you tried to restart the app a few times? If still not reproduceable, create a block and then restart 1 or 2 times...

@chimp1984
Copy link
Contributor Author

chimp1984 commented Nov 30, 2021

I think the issue is when the blocks request is faster then the blockhash requests from other nodes. So might be a bit of race conditions...
But to be sure that process is right:

  • Create some blocks while app is shut down
  • Start app (full mode/lite-monitor mode). should persiste latest block (triggered by hash response)
  • Restart
  • Does not parse as blockheight of persisted dao state is latest height, but start block calculated with +1 so the code for starting to parse ignores the request as requested block is in the future. One can check the logs at getStartBlockHeight in BsqNode and in startParseBlocks in FullNode. The relevant code is:
if (startBlockHeight <= chainHeight) {
      parseBlocksOnHeadHeight(startBlockHeight, chainHeight);
}

This is false in case the startBlockHeight is one block higher than the chainHeight.

@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 1, 2021

Following those steps above I can re-produce the problem 👍 and verify that this PR fixes the problem.
Still this PR causes the new problem mentioned here #5875 (comment)

@chimp1984
Copy link
Contributor Author

chimp1984 commented Dec 3, 2021

This change causes fresh Mainnet light nodes to finalize the block parsing until the next block is received. So they seem to be stuck on DAO Syncronization until a new block is received.

You mean a DAO lite mode with DAO hash monitoring mode set to lite mode as well?

Can you reproduce it or provide more info?

…leted yet.

- At genesis we use the genesis height for request (not height+1)
- If wallet is not synced yet we do not call onParseBlockChainComplete (as it was before)
@ripcurlx
Copy link
Contributor

ripcurlx commented Dec 6, 2021

I just tried to re-produce my issue with the current state of the PR and everything is looking fine now.

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK - Tested on Mainnet

@ripcurlx ripcurlx merged commit 6e1a805 into bisq-network:master Dec 6, 2021
@chimp1984 chimp1984 deleted the fix-incorrect-chainheight branch December 7, 2021 15:06
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

3 participants