Skip to content

max-bytes PR follow-up#2318

Merged
melekes merged 5 commits intodevelopfrom
anton/max-bytes-fixes
Sep 4, 2018
Merged

max-bytes PR follow-up#2318
melekes merged 5 commits intodevelopfrom
anton/max-bytes-fixes

Conversation

@melekes
Copy link
Contributor

@melekes melekes commented Sep 3, 2018

No description provided.

this mirrors ReapMaxBytes behavior

See #2184 (comment)
tested with:

```
func TestMaxAminoOverheadForBlock(t *testing.T) {

        maxChainID := ""
        for i := 0; i < MaxChainIDLen; i++ {
                maxChainID += "𠜎"
        }

        h := Header{
                ChainID:            maxChainID,
                Height:             10,
                Time:               time.Now().UTC(),
                NumTxs:             100,
                TotalTxs:           200,
                LastBlockID:        makeBlockID(make([]byte, 20), 300, make([]byte, 20)),
                LastCommitHash:     tmhash.Sum([]byte("last_commit_hash")),
                DataHash:           tmhash.Sum([]byte("data_hash")),
                ValidatorsHash:     tmhash.Sum([]byte("validators_hash")),
                NextValidatorsHash: tmhash.Sum([]byte("next_validators_hash")),
                ConsensusHash:      tmhash.Sum([]byte("consensus_hash")),
                AppHash:            tmhash.Sum([]byte("app_hash")),
                LastResultsHash:    tmhash.Sum([]byte("last_results_hash")),
                EvidenceHash:       tmhash.Sum([]byte("evidence_hash")),
                ProposerAddress:    tmhash.Sum([]byte("proposer_address")),
        }
        b := Block{
                Header:     h,
                Data:       Data{Txs: makeTxs(10000, 100)},
                Evidence:   EvidenceData{},
                LastCommit: &Commit{},
        }

        bz, err := cdc.MarshalBinary(b)
        require.NoError(t, err)

        assert.Equal(t, MaxHeaderBytes+MaxAminoOverheadForBlock-2, len(bz)-1000000-20000-1)
}
```
@melekes melekes requested review from ebuchman and xla as code owners September 3, 2018 09:33
@melekes melekes force-pushed the anton/max-bytes-fixes branch from fef6ab6 to 4da659d Compare September 3, 2018 10:47
@codecov-io
Copy link

codecov-io commented Sep 3, 2018

Codecov Report

Merging #2318 into develop will increase coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           develop   #2318      +/-   ##
==========================================
+ Coverage    60.88%   60.9%   +0.01%     
==========================================
  Files          197     197              
  Lines        16283   16273      -10     
==========================================
- Hits          9914    9911       -3     
+ Misses        5502    5497       -5     
+ Partials       867     865       -2
Impacted Files Coverage Δ
mempool/mempool.go 69.91% <0%> (+0.56%) ⬆️
node/node.go 64.09% <66.66%> (-0.1%) ⬇️
blockchain/pool.go 65.73% <0%> (-0.7%) ⬇️
evidence/wire.go 100% <0%> (ø) ⬆️
p2p/peer.go 59.09% <0%> (ø) ⬆️
consensus/state.go 76.93% <0%> (+0.23%) ⬆️
privval/socket_tcp.go 75% <0%> (+15%) ⬆️

@melekes melekes mentioned this pull request Sep 3, 2018
LastBlockID: makeBlockID(make([]byte, 20), 300, make([]byte, 20)),
NumTxs: math.MaxInt64,
TotalTxs: math.MaxInt64,
LastBlockID: makeBlockID(make([]byte, 20), math.MaxInt64, make([]byte, 20)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the 20 be changed to tmhash.Size?

mem.proxyMtx.Unlock()
// Filter sets a filter for mempool to only accept txs for which f(tx) returns
// true.
func Filter(f func(types.Tx) bool) MempoolOption {
Copy link
Contributor

Choose a reason for hiding this comment

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

(Sorry for the bikeshed) Can this still be named SetFilter? I feel like that was clearer, since this isn't the actual filter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis

I don't think SetFilter is a good name for an option (see the usage). Maybe WithFilter or FilterFunc?

Copy link
Contributor

Choose a reason for hiding this comment

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

WithFilter sounds better to me

Copy link
Contributor

@ValarDragon ValarDragon left a comment

Choose a reason for hiding this comment

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

LGTM!

@melekes melekes merged commit 166fd82 into develop Sep 4, 2018
@melekes melekes deleted the anton/max-bytes-fixes branch September 4, 2018 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants