Improvements to StreamReader

[dr-orlovsky]

StreamReader is now generalized to allow:

• reading not only data from network TcpStream, but any input stream, including files
• supporting Read, which is more performance-efficient than a raw read
• parses and returns not just RawMessage, but any serializable data type
• because of this, next_message function is renamed into read_next
• embedded stream is now boxed

Since the changes are compatibility-breaking, if the PR will be accepted, the version number has to be bumped accordingly.

Necessary testcase included. You can play as well with this minimalistic test parsing shortened version of the blk00000.dat file: https://github.com/pandoracore/bpdump

Code sample:

    let mut stream_reader = match filename {
        Some(name) => {
            let buf_read: Box<dyn io::Read> = Box::new(io::BufReader::new(File::open(name)?));
            StreamReader::new(buf_read, None)
        },
        None => {
            let stdin: Box<dyn io::Read> = Box::new(io::stdin());
            StreamReader::new(stdin, None)
        },
    };

    let mut blkno = 0;
    loop {
        // Reading magic number
        match stream_reader.read_next::<u32>() {
            Ok(0xD9B4BEF9) => (),
            _ => {
                eprintln!("No magic number found");
                break;
            }
        }
        // Skipping block length
        eprintln!("Magic number ok");
        let _ = stream_reader.read_next::<u32>()?;
        // Reading block
        match stream_reader.read_next::<Block>() {
            Err(err) => {
                eprintln!("{}", err);
                break;
            },
            Ok(block) => {
                eprintln!("* read block no {}, id {}", blkno, block.bitcoin_hash());
                println!("{:#?}", block.header);
                println!("{:#?}", block.txdata[0]);
                blkno += 1;
            },
        }
    }

[stevenroose]

Can't these two commits be squashed together?

• Making stream to be a ref in StreamReader
• Making stream to be a boxed in StreamReader

[dr-orlovsky]

Can't these two commits be squashed together?

done

[dr-orlovsky]

Its strange that rust nightly, stable and beta are building, but 1.22 fails to solve cargo dependency on rand package (which I didn't touched at all). Any ideas what can cause this?

[elichai]

Because you reintroduced tempfile

[elichai]

NACK #306

Everything you plan to do with a file can be done in memory via a buffer(vec/iter something else)

[dr-orlovsky]

Everything you plan to do with a file can be done in memory via a buffer(vec/iter something else)

@elichai got it. It was introduced to mimic real-world file reading in tests as much as possible, but it is clearly not needed with that price of security

[codecov-io]

Codecov Report

Merging #318 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #318      +/-   ##
==========================================
- Coverage   81.93%   81.93%   -0.01%     
==========================================
  Files          38       38              
  Lines        6982     6981       -1     
==========================================
- Hits         5721     5720       -1     
  Misses       1261     1261

Impacted Files	Coverage Δ
src/network/stream_reader.rs	95.62% <100%> (+1.22%)	⬆️
src/util/misc.rs	96.77% <0%> (-0.37%)	⬇️
src/blockdata/script.rs	78.31% <0%> (-0.29%)	⬇️
src/blockdata/transaction.rs	93.57% <0%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24361dd...749251e. Read the comment docs.

[dr-orlovsky]

@apoelstra @elichai all suggestions are accepted and added to the PR; commits were squashed; so now its ready for your re-review

[apoelstra]

Much nicer :)

[elichai]

ACK 749251e looks good :)

[dr-orlovsky]

@elichai I assume you need to complete review with a Github button in order to get this merged?

[dr-orlovsky]

Seems we are ready to merge this PR, all comments were addressed and two reviewers had ACK. What is the next step?

[apoelstra]

We need another reviewer with merge access to ACK.

[dr-orlovsky]

@elichai it looks like we require your ACK to merge the stuff...