Add debug getbadblocks #9207
Add debug getbadblocks #9207
Conversation
Updated the traits to allow for endpoints to return a result of badblocks that are held in the blockchain implementation.
|
It looks like @cpurta hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io Once you've signed, please reply to this thread with Many thanks, Parity Technologies CLA Bot |
|
[clabot:check] |
|
It looks like @cpurta signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
|
This currently does not build as of now. Issue is stemming from I am still pretty new to Rust so there are probably some lines that can be refactored. Also if the naming of the new functions seems off I can rename those. |
5chdn
added
A3-inprogress ⏳
|
Any chance that I can get a quick review on the changes here and some feedback on how to finish this? |
|
Let me blindly ping @andresilva @debris @tomusdrw to attract their attention :) |
Fixed the bad_blocks function by correctly unwrapping and chaining together functions to create RichBlocks from the hashes of the current bad_blocks on the chain.
| @@ -985,6 +987,11 @@ impl BlockChain { | |||
|
|
|||
| let info = self.block_info(&header, route, &extras); | |||
|
|
|||
| if self.is_blacklisted_hash(hash) { | |||
| self.bad_blocks.write().insert(hash, info); | |||
There was a problem hiding this comment.
Can you describe how this lock is used? Locks are generally bad and increase complexity, but if its use and pattern is straightforward then it can obviously be completely fine.
There was a problem hiding this comment.
The reason behind using a lock for those hashes is due to the potential for a blockchain client to try and read what the current bad blocks are. At the same time a bad block could be inserted into our hashmap of bad blocks. Thus a race condition for the same resource so it made sense to have a lock.
| /// Get all blacklisted hashes | ||
| fn blacklisted_hashes(&self) -> HashMap<H256, bool> { | ||
| let mut blacklist = HashMap::new(); | ||
| blacklist.insert("05bef30ef572270f654746da22639a7a0c97dd97a7050b9e252391996aaeb689".into(), true); |
There was a problem hiding this comment.
What are these and why are they inserted here?
There was a problem hiding this comment.
These represent the list of bad hashes (usually hard forks) that determine whether a block is considered "bad". Similar to the implementation in geth: https://github.com/ethereum/go-ethereum/blob/master/core/blocks.go
rpc/src/v1/impls/eth.rs
Outdated
| @@ -817,6 +817,16 @@ impl<C, SN: ?Sized, S: ?Sized, M, EM, T: StateInfo + 'static> Eth for EthClient< | |||
| Ok(true) | |||
| } | |||
|
|
|||
| fn bad_blocks(&self, include_txs: bool) -> BoxFuture<Vec<RichBlock>> { | |||
| let blocks = self.client.bad_blocks() | |||
| .unwrap() | |||
There was a problem hiding this comment.
Please see our contribution guidelines regarding unwrap() here https://wiki.parity.io/Coding-guide.
Basically at no point in any code should unwrap() ever be called.
There was a problem hiding this comment.
Ahh. I didn't see that guide. I'll get on fixing that.
|
@folsen addressed the unwrap issue and commented back on a couple of your comments. Would appreciate some feedback on comments and changes when you have some time. |
5chdn
added
A0-pleasereview 🤓
|
Superseeded by #9433 |
Blockchain implementation will now have a blacklist of hashes that will allow for it to add bad blocks to a HashMap and in order to get all bad blocks we return a vector of hashes. This then allows for the rpc api to then return all of the current bad blocks that are within the current blockchain.
This will resolve #8761 when merged.