Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Make block generator easier to use #7888

Merged
merged 7 commits into from
Feb 16, 2018
Merged

Make block generator easier to use #7888

merged 7 commits into from
Feb 16, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Feb 14, 2018

let's make it easier to write tests for blockchain

@debris debris added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust. labels Feb 14, 2018
let b5a = canon_chain.generate(&mut finalizer).unwrap();
let genesis = BlockBuilder::genesis();
let b1a = genesis.add_block();
let b2a = b1a.add_block();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not:

let chain_a = genesis.add_blocks(5);
let chain_b = genesis.add_blocks_with_difficulty(9);

let uncle_headers = chain_b[2..4].rev().map(|x| x.last_block_header()).collect();

or

let chain_a = genesis.add_blocks(5);
// with_difficulty clones the builder and changes the default options for all new generated blocks
let chain_b = genesis.with_difficulty(9).add_blocks(5);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if you look closely, there are 5 different forks :)

genesis
|      \ 
b1a     b1b
|   \
|    \
|     \
|      \
b2a     b2b
|
....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't want to change the logic of a test

let first = canon_chain.generate(&mut finalizer).unwrap();
let genesis = BlockBuilder::genesis();
let first = genesis.add_block();
let mut generator = BlockGenerator::new(vec![genesis, first]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO BlockGenerator should not be Iterator, but rather IntoIterator (Maybe even call it BlockChain)

let generator = BlockGenerator::new(&[genesis, first]);
new_chain(&generator[0].data())
// Retrieve iterator
let mut it = generator.iter();

// or consume generator and do sth with blocks
for b in generator {
  ...
}

}

#[inline]
pub fn data(&self) -> Bytes {
Copy link
Collaborator

Choose a reason for hiding this comment

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

data is imho too generic. Let's just call it encoded() maybe?

}

#[derive(Debug)]
pub struct BlockMetadata {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would rather call it BlockOptions

}

#[inline]
pub fn last_block_hash(&self) -> H256 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Imho block is redundant, maybe do this:

blocks.last().number()
blocks.last().hash()
blocks.last().header()
blocks.last().into::<Block>()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 , if blocks.last() is already a Block, there is no need for into :)

@5chdn 5chdn added this to the 1.10 milestone Feb 14, 2018
@svyatonik
Copy link
Collaborator

Test failure was fixed in master => merged master here

Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Looks good.
Although imho still would be better to have BlockGenerator behave more like a Vec. Advantages:

  1. Can be iterated multiple times (should implement IntoIterator and .iter())
  2. No need for funny generator.next().unwrap(), but you rather get nice generator[0] that panics if item is not there.

Actually why not make BlockGenerator::new() just return Vec?

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 15, 2018
@debris
Copy link
Collaborator Author

debris commented Feb 15, 2018

I removed usages of generator.next().unwrap() :)

@5chdn 5chdn merged commit c6b9317 into master Feb 16, 2018
@5chdn 5chdn deleted the block_generator_cleanup branch February 16, 2018 09:11
tomusdrw pushed a commit that referenced this pull request Feb 28, 2018
* Make block generator easier to use

* applied review suggestions

* rename BlockMetadata -> BlockOptions

* removed redundant uses of blockchain generator and genereator.next().unwrap() calls
@5chdn 5chdn added the B0-patch label Feb 28, 2018
5chdn pushed a commit that referenced this pull request Feb 28, 2018
* Hardware-wallet/usb-subscribe-refactor (#7860)

* Hardware-wallet fix

* More fine-grained initilization of callbacks by vendorID, productID and usb class
* Each device manufacturer gets a seperate handle thread each
* Replaced "dummy for loop" with a delay to wait for the device to boot-up properly
* Haven't been very carefully with checking dependencies cycles etc
* Inline comments explaining where shortcuts have been taken
* Need to test this on Windows machine and with Ledger (both models)

Signed-off-by: niklasad1 <niklasadolfsson1@gmail.com>

* Validate product_id of detected ledger devices

* closed_device => unlocked_device

* address comments

* add target in debug

* Address feedback

* Remove thread joining in HardwareWalletManager
* Remove thread handlers in HardwareWalletManager because this makes them unused

* fixed broken logs (#7934)

* fixed broken logs

* bring back old lock order

* removed bloom groups from blockchain

* revert unrelated changes

* simplify blockchain_block_blooms

* Bump WS (#7952)

* Calculate proper keccak256/sha3 using parity. (#7953)

* Increase max download limit to 128MB (#7965)

* fetch: increase max download limit to 64MB

* parity: increase download size limit for updater service

* Detect too large packets in snapshot sync. (#7977)

* fix traces, removed bloomchain crate, closes #7228, closes #7167 (#7979)

* Remvoe generator.rs

* Make block generator easier to use (#7888)

* Make block generator easier to use

* applied review suggestions

* rename BlockMetadata -> BlockOptions

* removed redundant uses of blockchain generator and genereator.next().unwrap() calls
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants