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.

Sign up for GitHub

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

Jump to bottom

BlockValidator can sometimes drop responders without responding #4619

Closed
SaiProServ opened this issue Mar 20, 2024 · 1 comment
Closed

BlockValidator can sometimes drop responders without responding #4619

SaiProServ opened this issue Mar 20, 2024 · 1 comment
Assignees
@fizyk20
Labels
bug Something isn't working

Comments

@SaiProServ
Copy link

If the BlockValidator receives two requests for validating the same block in quick succession (a valid scenario if we learn of new holders of the same proposal), it is possible that the second one will arrive before the state for the first one is inserted into validation_states, in which case both will be processed as new requests and the responder and holder for the first one will end up being dropped.
@SaiProServ SaiProServ added the bug Something isn't working label Mar 20, 2024
@darktemplate
Copy link

Hi Vibrant Casper's Dev community ! 💯
i hope i'm not disrispect or trolling by commenting this particular issue but it rememeber this particular point being discussed (and published at IOHK). Here is basically how they handle it there :

_In the Cardano blockchain, the BlockValidator is designed to handle such scenarios to prevent the issue you described. Here's a simplified explanation of how it might be handled:

  1. Concurrency Control: The BlockValidator is likely designed to handle concurrent requests. This means that even if two requests for validating the same block arrive in quick succession, they won't interfere with each other's processing.

  2. State Management: The validation_states you mentioned is likely a data structure used to keep track of the validation process for each block. When a new request comes in, the BlockValidator would first check this data structure to see if the block is already being processed. If it is, the new request would be queued or discarded, depending on the specific implementation.

  3. Unique Identifiers: Each block validation request might have a unique identifier associated with it. This could be the block hash or some other unique value. This identifier would be used to check if a request is already being processed, as mentioned in the previous point.

  4. Responder and Holder Management: The responder and holder for each request would be managed in such a way that they aren't dropped until the validation process is complete. This could be done by keeping references to them in the validation_states data structure, for example.

In the specific case of Cardano, the Ouroboros protocol, which is the consensus mechanism used by Cardano, is designed to handle such scenarios. However, the exact details of how this is done would depend on the specific implementation of the BlockValidator and the rest of the Cardano node software. For the most accurate information, it would be best to refer to the official Cardano documentation or the source code of the Cardano node software._

May it help/inspires @SaiProServ . If not i send all my luck and best wishes to hard working dev and Casper's community.

Best,

Simon

@fizyk20 fizyk20 mentioned this issue Apr 5, 2024
Merged
casperlabs-bors-ng bot added a commit that referenced this issue Apr 9, 2024
@casperlabs-bors-ng @sacherjj
@EdHastingsCasperAssociation @fizyk20
Merge #4649 #4650
91f15c6 
4649: casper_updater compatibility with Condor and version bumps r=sacherjj a=sacherjj

Removed chainspec.toml updates as we do not use for activation point.
Added binary_port
Removed json_rpc
Corrected for reorganization
Bumping versions for release

4650: Fix a bug in the BlockValidator r=fizyk20 a=fizyk20

Closes #4619 

This is a simple, slightly hacky approach to solving the issue - but a less hacky one involving adding more variants to `BlockValidationState` and storing the state immediately after receiving a request turned out to become quite complex, which is why I chose this way eventually.


Co-authored-by: Rafał Chabowski <rafal@casperlabs.io>
Co-authored-by: Joe Sacher <321623+sacherjj@users.noreply.github.com>
Co-authored-by: casperlabs-bors-ng[bot] <82463608+casperlabs-bors-ng[bot]@users.noreply.github.com>
Co-authored-by: Ed Hastings <ed@casperlabs.io>
Co-authored-by: Alexandru Sardan <alexandru@casperlabs.io>
Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io>
casperlabs-bors-ng bot added a commit that referenced this issue Apr 9, 2024
@casperlabs-bors-ng @jacek-casper @fizyk20
Merge #4645 #4650 #4653
1872a87 
4645: Binary port balance query r=jacek-casper a=jacek-casper

This PR adds a new request to the binary port that allows querying for balance directly.
- I've added some additional types to `binary_port`, they're very similar to the existing `storage` types, with some slight adjustments to adapt them to the binary port:
  - added `BalanceResponse` for the new response payload type
  - added `PurseIdentifier` for specifying what balance to query, it's basically `BalanceIdentifier`, but without PenalizedAccount and Internal variants (these two look like they're for internal use only)
  - added `GlobalStateRequest::BalanceByStateRoot`, which has a state identifier, a purse identifier and a timestamp for holds lookup, it uses a `Timestamp` unlike the `storage` types which use `HoldsEpoch`, because `HoldsEpoch` requires a chainspec value to construct (balance hold interval)
  - added `GlobalStateRequest::BalanceByBlock`, which has a block identifier and a purse identifier, the holds timestamp is derived from the block

Also:
- boxed the `GetRequest::State` variant because it started triggering a lint
- added some missing handling for `WasmV1Result`

Related sidecar changes: casper-network/casper-sidecar#274

4650: Fix a bug in the BlockValidator r=fizyk20 a=fizyk20

Closes #4619 

This is a simple, slightly hacky approach to solving the issue - but a less hacky one involving adding more variants to `BlockValidationState` and storing the state immediately after receiving a request turned out to become quite complex, which is why I chose this way eventually.


4653: Use custom Serialize/Deserialize for EntityAddr r=jacek-casper a=jacek-casper

The existing Serialize/Deserialize impls encode the address as a JSON array of bytes instead of the formatted string format. This PR fixes that.

There was also an issue with the schema, which I addressed here as well.

Co-authored-by: Jacek Malec <145967538+jacek-casper@users.noreply.github.com>
Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io>
casperlabs-bors-ng bot added a commit that referenced this issue Apr 9, 2024
@casperlabs-bors-ng @jacek-casper @fizyk20
Merge #4645 #4650
3707a4f 
4645: Binary port balance query r=jacek-casper a=jacek-casper

This PR adds a new request to the binary port that allows querying for balance directly.
- I've added some additional types to `binary_port`, they're very similar to the existing `storage` types, with some slight adjustments to adapt them to the binary port:
  - added `BalanceResponse` for the new response payload type
  - added `PurseIdentifier` for specifying what balance to query, it's basically `BalanceIdentifier`, but without PenalizedAccount and Internal variants (these two look like they're for internal use only)
  - added `GlobalStateRequest::BalanceByStateRoot`, which has a state identifier, a purse identifier and a timestamp for holds lookup, it uses a `Timestamp` unlike the `storage` types which use `HoldsEpoch`, because `HoldsEpoch` requires a chainspec value to construct (balance hold interval)
  - added `GlobalStateRequest::BalanceByBlock`, which has a block identifier and a purse identifier, the holds timestamp is derived from the block

Also:
- boxed the `GetRequest::State` variant because it started triggering a lint
- added some missing handling for `WasmV1Result`

Related sidecar changes: casper-network/casper-sidecar#274

4650: Fix a bug in the BlockValidator r=fizyk20 a=fizyk20

Closes #4619 

This is a simple, slightly hacky approach to solving the issue - but a less hacky one involving adding more variants to `BlockValidationState` and storing the state immediately after receiving a request turned out to become quite complex, which is why I chose this way eventually.


Co-authored-by: Jacek Malec <145967538+jacek-casper@users.noreply.github.com>
Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io>
casperlabs-bors-ng bot added a commit that referenced this issue Apr 10, 2024
@casperlabs-bors-ng @fizyk20
Merge #4650
4b9da95 
4650: Fix a bug in the BlockValidator r=fizyk20 a=fizyk20

Closes #4619 

This is a simple, slightly hacky approach to solving the issue - but a less hacky one involving adding more variants to `BlockValidationState` and storing the state immediately after receiving a request turned out to become quite complex, which is why I chose this way eventually.


Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io>
casperlabs-bors-ng bot added a commit that referenced this issue Apr 10, 2024
@casperlabs-bors-ng @fizyk20
Merge #4650
e84758a 
4650: Fix a bug in the BlockValidator r=fizyk20 a=fizyk20

Closes #4619 

This is a simple, slightly hacky approach to solving the issue - but a less hacky one involving adding more variants to `BlockValidationState` and storing the state immediately after receiving a request turned out to become quite complex, which is why I chose this way eventually.


Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io>
@fizyk20 fizyk20 closed this as completed May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fizyk20 fizyk20

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fizyk20 @darktemplate @SaiProServ