util: CBufferedFile fixes #4265
Conversation
str4d
left a comment
str4d
left a comment
There was a problem hiding this comment.
There is very comprehensive review in the upstream PR (nice work @LarryRuane!)
One blocking comment about the license header, otherwise utACK.
src/test/streams_tests.cpp
Outdated
There was a problem hiding this comment.
This line (and similar) differ from the upstream commit because we have not cherry-picked bitcoin/bitcoin#9902.
src/test/streams_tests.cpp
Outdated
There was a problem hiding this comment.
This PR uses GetRandInt instead of InsecureRandRange because we have not cherry-picked bitcoin/bitcoin#10321.
src/test/streams_tests.cpp
Outdated
There was a problem hiding this comment.
These differ from the upstream PR because we haven't cherry-picked bitcoin/bitcoin#12740.
src/test/streams_tests.cpp
Outdated
There was a problem hiding this comment.
This uses 50 iterations in the upstream PR, but if this isn't too slow then I'm fine with more iterations here.
There was a problem hiding this comment.
This was an oversight on my part; I wanted to do a little extra testing and forgot to change it back to 50. The time (both CPU and real) to run this test is about 1.4s with 50 iterations, and 1.7s with 500 iterations. It's probably worth the 0.3 seconds to get better test coverage, so I'll leave it at 500.
src/streams.h
Outdated
There was a problem hiding this comment.
These changes are in this PR because we haven't cherry-picked bitcoin/bitcoin#8808.
LarryRuane
force-pushed
the
CBufferedFile-fixes
branch
from
dc7e51c to
0030574
Compare
There was a problem hiding this comment.
Thank you for the review, @str4d, force-pushed (but no rebase) to fix the one blocking suggestion.
src/test/streams_tests.cpp
Outdated
There was a problem hiding this comment.
This was an oversight on my part; I wanted to do a little extra testing and forgot to change it back to 50. The time (both CPU and real) to run this test is about 1.4s with 50 iterations, and 1.7s with 500 iterations. It's probably worth the 0.3 seconds to get better test coverage, so I'll leave it at 500.
str4d
added
the
S-scratching-an-itch
|
📌 Commit 0030574 has been approved by |
|
⌛ Testing commit 0030574fdea546e34bf45fb7b20b92ff72f86264 with merge 28e0a729959b7670d22cee6ae124757dddf63ddb... |
|
💔 Test failed - pr-merge |
There was a problem hiding this comment.
Test failed because #3858 was merged, which pulled in bitcoin/bitcoin#8914. Please rebase the PR.
LarryRuane
force-pushed
the
CBufferedFile-fixes
branch
from
0030574 to
fd94e27
Compare
LarryRuane
force-pushed
the
CBufferedFile-fixes
branch
from
fd94e27 to
75251cb
Compare
|
@zkbot r+ |
|
📌 Commit 75251cb has been approved by |
bitcoin/bitcoin#16577 zcash/zcash#4265 - added CBufferedFile fix (thx LarryRuane) - added unit test (streams_tests.cpp rewritten for Google C++ Testing Framework - additional komodo_block_load test (read 2 blocks from binary file) - removed CBufferedFile::SetAnyPos method (had incorrect logic) - removed komodo_index2pubkey33, komodo_blockload (as not-used and related to SetAnyPos) -
- added CBufferedFile fix (thx LarryRuane) - added unit test (streams_tests.cpp rewritten for Google C++ Testing Framework) - additional komodo_block_load test (read 2 blocks from binary file) bitcoin/bitcoin#16577 zcash#4265 DeckerSU/KomodoOcean@9700247
3 participants
Cherry-pick bitcoin/bitcoin#16577