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

Mempool: Transaction Reintroduction #162

Merged
merged 65 commits into from May 21, 2019

Conversation

Projects
None yet
4 participants
@marklnichols
Copy link
Contributor

commented Apr 22, 2019

No description provided.

gregorycollins and others added some commits Apr 8, 2019

WIP
WIP
WIP
WIP
WIP

@marklnichols marklnichols marked this pull request as ready for review May 14, 2019

@marklnichols marklnichols requested review from larskuhtz and slpopejoy May 14, 2019

@marklnichols marklnichols changed the title Mark/mempool consensus Mark/mempool consensus - please do not merge until tested in TestNet May 14, 2019

Show resolved Hide resolved src/Chainweb/BlockHeaderDB.hs Outdated
Right (LeftD blk, strm) -> go strm (V.cons blk oldBlocks, newBlocks)
Right (RightD blk, strm) -> go strm (oldBlocks, V.cons blk newBlocks)
Right (BothD lBlk rBlk, strm) -> go strm ( V.cons lBlk oldBlocks,
V.cons rBlk newBlocks )

This comment has been minimized.

Copy link
@fosskers

fosskers May 14, 2019

Contributor

consing like this gets expensive quickly.

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 16, 2019

Author Contributor

This is list is just the depth of a fork, so it should be extremely short.

This comment has been minimized.

Copy link
@fosskers

fosskers May 21, 2019

Contributor

Is Vector being used for something efficient later in the pipeline?

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 21, 2019

Author Contributor

The use of Vector here is due to it being the collection used by the mempool, e.g.
mempoolInsert :: Vector t -> IO ()
mempoolGetBlock :: Int64 -> IO (Vector t)

I assume Greg made an appropriate choice here, but I don't know if there were any particular concerns about performance that led to it.

This comment has been minimized.

Copy link
@fosskers

fosskers May 21, 2019

Contributor

One would think that the consing here could be done via a list, with a V.fromList at the very end. But if the list is always™ going to be short, then the change might not be worth it either.

Show resolved Hide resolved src/Chainweb/Mempool/InMem.hs Outdated
toJSON (GasPrice d) = toJSON d

instance FromJSON GasPrice where
parseJSON v = GasPrice <$> parseJSON v

This comment has been minimized.

Copy link
@fosskers

fosskers May 14, 2019

Contributor

Do these live in Pact yet?

This comment has been minimized.

Copy link
@marklnichols

marklnichols May 16, 2019

Author Contributor

No, I don't see them in Pact. I can move them over in a quick follow-up PR. I don't want to add a Pact version bump on top of this already giant PR.

Show resolved Hide resolved src/Chainweb/Payload/PayloadStore/InMemory.hs Outdated
Show resolved Hide resolved test/Chainweb/Test/Mempool/InMem.hs Outdated
Show resolved Hide resolved test/Chainweb/Test/Mempool/Socket.hs Outdated
Show resolved Hide resolved test/Chainweb/Test/Mempool/Socket.hs Outdated
Show resolved Hide resolved test/Chainweb/Test/Mempool/Sync.hs Outdated

marklnichols added some commits May 15, 2019

@fosskers fosskers changed the title Mark/mempool consensus - please do not merge until tested in TestNet Mempool: Transaction Reintroduction May 21, 2019

@marklnichols marklnichols merged commit d6bb45e into master May 21, 2019

1 check passed

ci/gitlab/gitlab.com Pipeline passed on GitLab
Details
Show resolved Hide resolved chainweb.cabal
Show resolved Hide resolved src/Chainweb/Mempool/InMem.hs
Show resolved Hide resolved src/Chainweb/Mempool/Consensus.hs
Show resolved Hide resolved src/Chainweb/Mempool/Consensus.hs
Show resolved Hide resolved src/Chainweb/Mempool/InMem.hs
Show resolved Hide resolved src/Chainweb/Mempool/Mempool.hs
Show resolved Hide resolved src/Chainweb/Transaction.hs
Show resolved Hide resolved src/Chainweb/Chainweb/ChainResources.hs
Show resolved Hide resolved src/Chainweb/Mempool/Consensus.hs
Show resolved Hide resolved test/Chainweb/Test/Mempool/Consensus.hs

@marklnichols marklnichols deleted the mark/mempool-consensus branch May 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.