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

execNewBlock: remove the parent header as parameter and sync from the db instead #1741

Closed
wants to merge 2 commits into from
Closed

execNewBlock: remove the parent header as parameter and sync from the db instead #1741

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 26, 2023 •
edited by ghost
Loading

@ghost ghost marked this pull request as ready for review September 26, 2023 17:39
@ghost ghost requested review from edmundnoble and chessai September 26, 2023 17:39
edmundnoble
edmundnoble reviewed Sep 26, 2023
View reviewed changes
Copy link
Contributor

@edmundnoble edmundnoble left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This can't be merged until after the release. All of my comments are pedantic stuff which barely matters, I'm extremely impressed that we can get away with this, it looks exactly like I'd expect. We need to test this on a real mining node as well before merge.

test/Chainweb/Test/Pact/ModuleCacheOnRestart.hs Outdated
Comment on lines 272 to 276
getPWOByHeader :: BlockHeader -> TestBlockDb -> IO PayloadWithOutputs
getPWOByHeader h (TestBlockDb _ pdb _) = do
pwo <- casLookupM pdb (_blockPayloadHash h)
return pwo

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be = casLookupM pdb (_blockPayloadHash h)

test/Chainweb/Test/Pact/PactMultiChainTest.hs Outdated
Comment on lines 1424 to 1425
pwo <- liftIO $ casLookupM pdb (_blockPayloadHash h)
return pwo
Copy link
Contributor

Choose a reason for hiding this comment

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

Can just replace this by liftIO $ casLookupM pdb (_blockPayloadHash h)

Copy link
Contributor

@edmundnoble edmundnoble Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hell maybe this should be a shared utility. ModuleCacheOnRestart wants it too.

test/Chainweb/Test/Pact/PactMultiChainTest.hs Outdated
@@ -1404,6 +1418,12 @@ getPWO chid = do
pwo <- liftIO $ casLookupM pdb (_blockPayloadHash h)
return (pwo,h)

getPWOByHeader :: BlockHeader -> PactTestM PayloadWithOutputs
getPWOByHeader h = do
(TestBlockDb _ pdb _) <- view menvBdb
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for these parens

@edmundnoble edmundnoble changed the title execNewBlock: remove the parent header as paramater and sync from the db instead execNewBlock: remove the parent header as parameter and sync from the db instead Oct 4, 2023
@ghost ghost force-pushed the execnewblock-parentheader branch from 83a638f to 7b74390 Compare October 12, 2023 16:31
@ghost ghost requested a review from edmundnoble October 12, 2023 16:31
@ghost ghost force-pushed the execnewblock-parentheader branch from 93ce2f2 to 3349023 Compare October 17, 2023 12:31
@edmundnoble
Copy link
Contributor

We actually don't really need to test this on a real mining node, fake mining with a simulated time distribution also works, as long as it's in a network with multiple mining nodes.

@ghost
Copy link
Author

ghost commented Oct 19, 2023

@edmundnoble will wait until the master is fixed so we can merge with green CI this PR? Or can merge already? it was green before

@edmundnoble
Copy link
Contributor

@ak3n I want this PR benchmarked to see how many blocks we produce with a simulated miner first. I can help with that some time when you're free.

@edmundnoble
Copy link
Contributor

Actually we should probably use a load test for this benchmark.

@ghost ghost force-pushed the execnewblock-parentheader branch 2 times, most recently from 4d7ac96 to a814296 Compare October 30, 2023 09:55
@ghost ghost force-pushed the execnewblock-parentheader branch 2 times, most recently from 8a80f6b to d3daded Compare November 3, 2023 11:56
@ghost ghost force-pushed the execnewblock-parentheader branch from d3daded to 7244f32 Compare November 14, 2023 08:33
0xd34df00d
0xd34df00d approved these changes Nov 14, 2023
View reviewed changes
Copy link

@0xd34df00d 0xd34df00d left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The logic looks good as per discussion, so I only have a few minor nits.

More eyes on this are definitely welcome, though.

bench/Chainweb/Pact/Backend/ForkingBench.hs
let (T3 _ join1 pwo1) = mainLineBlocks !! 5

-- we have to execValidateBlock on `join1` block height to update the parent header
void $ validateBlock join1 (payloadWithOutputsToPayloadData pwo1) pactQueue
Copy link

Choose a reason for hiding this comment

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

You might create a helper function (for the tests only!) like

updateParentHeader join pwo queue = void $ validateBlock join (payloadWithOutputsToPayloadData pwo) queue

avoiding the comments in the tests and making the tests a bit more expressive.

src/Chainweb/Pact/PactService.hs Outdated
tryOne "execNewBlock" _newResultVar $
execNewBlock memPoolAccess _newBlockHeader _newMiner
() 1 $
tryOne "execNewBlock" _newResultVar $ do
Copy link

Choose a reason for hiding this comment

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

Tiny nit: this do isn't necessary.

src/Chainweb/Pact/Service/Types.hs Outdated
, _newResultVar :: !(PactExMVar PayloadWithOutputs)
}
instance Show NewBlockReq where show NewBlockReq{..} = show (_newBlockHeader, _newMiner)
instance Show NewBlockReq where show NewBlockReq{..} = show (_newMiner)
Copy link

Choose a reason for hiding this comment

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

The parens are probably no longer needed.

@ghost ghost force-pushed the execnewblock-parentheader branch from 7244f32 to 1a4ee00 Compare November 17, 2023 08:30
@ghost ghost force-pushed the execnewblock-parentheader branch from 9bb4573 to 619de66 Compare December 4, 2023 10:44
@ghost ghost force-pushed the execnewblock-parentheader branch from 619de66 to 01b4531 Compare December 21, 2023 14:19
@ghost
Copy link
Author

ghost commented Feb 12, 2024

Merged in #1803

@ghost ghost closed this Feb 12, 2024
@ghost ghost deleted the execnewblock-parentheader branch February 12, 2024 09:47
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chessai chessai Awaiting requested review from chessai

@edmundnoble edmundnoble Awaiting requested review from edmundnoble

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@edmundnoble @0xd34df00d