Skip to content

Fix fundemental BoltDB backend correctness issues.#3720

Merged
liamsi merged 2 commits intotendermint:developfrom
Yawning:boltdb-fixes
Jun 7, 2019
Merged

Fix fundemental BoltDB backend correctness issues.#3720
liamsi merged 2 commits intotendermint:developfrom
Yawning:boltdb-fixes

Conversation

@Yawning
Copy link
Contributor

@Yawning Yawning commented Jun 7, 2019

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG_PENDING.md

These changes fix #3717 and #3718.

@Yawning Yawning requested review from ebuchman, melekes and xla as code owners June 7, 2019 07:36
@codecov-io
Copy link

codecov-io commented Jun 7, 2019

Codecov Report

Merging #3720 into develop will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #3720      +/-   ##
===========================================
+ Coverage     63.8%   63.81%   +0.01%     
===========================================
  Files          241      241              
  Lines        19980    19980              
===========================================
+ Hits         12748    12750       +2     
+ Misses        6182     6179       -3     
- Partials      1050     1051       +1
Impacted Files Coverage Δ
types/tx.go 86.95% <0%> (-4.35%) ⬇️
blockchain/pool.go 79.6% <0%> (-0.66%) ⬇️
p2p/pex/pex_reactor.go 82.64% <0%> (-0.59%) ⬇️
consensus/reactor.go 70.77% <0%> (+0.58%) ⬆️
privval/signer_service_endpoint.go 89.09% <0%> (+5.45%) ⬆️

b := tx.Bucket(bucket)
for _, elem := range bdb.buffer {
if putErr := b.Put(elem.k, elem.v); putErr != nil {
return putErr
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch. How didn't we notice this during review 🤦‍♂ Thanks!

Copy link
Contributor

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you 👍

* [rpc] [\#3686](https://github.com/tendermint/tendermint/pull/3686) `HTTPClient#Call` returns wrapped errors, so a caller could use `errors.Cause` to retrieve an error code.

### BUG FIXES:
- [libs/db] Fixed the BoltDB backend's Batch.Delete implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [libs/db] Fixed the BoltDB backend's Batch.Delete implementation
- [libs/db] Fixed the BoltDB backend's Batch.Delete implementation (@Yawning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the force-pushed branch.


### BUG FIXES:
- [libs/db] Fixed the BoltDB backend's Batch.Delete implementation
- [libs/db] Fixed the BoltDB backend's Get and Iterator implementation
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [libs/db] Fixed the BoltDB backend's Get and Iterator implementation
- [libs/db] Fixed the BoltDB backend's Get and Iterator implementation (@Yawning)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the force-pushed branch.

Yawning added 2 commits June 7, 2019 08:26
`Delete` should queue a delete operation that targets the entire
database, and not just keys that happen to be in the current batch.
BoltDB's accessors will return slices that are only valid for the
lifetime of the transaction.  This adds copies where required to prevent
hard to debug crashes (among other things).
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

amazing 🔥 thank you!

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.

4 participants