Skip to content

Integrate bitcoin_hashes, remove rust-crypto and fuzz_util#215

Merged
dongcarl merged 7 commits intorust-bitcoin:masterfrom
dongcarl:2018-12-integrate-bitcoin-hashes
Jan 25, 2019
Merged

Integrate bitcoin_hashes, remove rust-crypto and fuzz_util#215
dongcarl merged 7 commits intorust-bitcoin:masterfrom
dongcarl:2018-12-integrate-bitcoin-hashes

Conversation

@dongcarl
Copy link
Copy Markdown
Member

No description provided.

@dongcarl dongcarl changed the title Integrate bitcoin_hashes, remove rust-crypto, deprecate fuzz_util Integrate bitcoin_hashes, remove rust-crypto and fuzz_util Jan 16, 2019
@dongcarl dongcarl force-pushed the 2018-12-integrate-bitcoin-hashes branch 3 times, most recently from 1607f2b to 09c643e Compare January 17, 2019 17:26
@dongcarl
Copy link
Copy Markdown
Member Author

I would like help debugging this from someone who is more familiar with serde and strason.

It would seem that running with --features=use-serde-decimal panics. git-bisect tells me that 9fc0680 is the offender.

@dongcarl
Copy link
Copy Markdown
Member Author

Attention Reviewers: I have identified the cause of the panic in Travis and fixed it in rust-bitcoin/bitcoin_hashes#29.

ariard
ariard previously approved these changes Jan 24, 2019
Copy link
Copy Markdown
Contributor

@ariard ariard left a comment

Choose a reason for hiding this comment

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

Nice job, just few remarks but I think it's more about rust-bitcoin usages than anything else.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why you not put use statement at file root ? You're doing it elsewhere, rust-bitcoin idiom ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm doing this too if the usage of the symbol is mostly local. It also makes moving functions between modules much easier.

Do you see any problems with doing it that way or is it just unusual?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We just don't do that for rust-lightning and never(rarely?) see it in general rust projects. But yeah function moving between modules is a good point, maybe you should stick to symbol strictly local ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you dereference required_target you can avoid to take reference both at target and hash below, but don't know what's best ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't make a huge difference here, as dereferencing required_target will call Copy, and that is arguably even more expensive.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes was a nit, just to be fixed, that something happening elsewhere in rust-bitcoin

@dongcarl
Copy link
Copy Markdown
Member Author

Going to push rebased version with bitcoin_hashes bumped to 0.3.0.

@dongcarl dongcarl force-pushed the 2018-12-integrate-bitcoin-hashes branch from 09c643e to f28757a Compare January 24, 2019 01:57
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

Except the one unsafe the PR looks good superficially, but I still have to take a closer look, it's so huuuuge.

Because features and dependencies share the same namespace, and we want
to pass down the optional dependence on serde to bitcoin_hashes, we need
to rename the feature to something other than serde. Right now only
features can be passed down to dependencies.

Note that we could have also renamed the dependency to something like
serde-dep and kept the same feature name, however, dependency renaming
has only been available since cargo 0.27.0

Features that represent optional dependencies have been prefixed with
'use-'. The travis file has also been modified to conform to this
change.
@dongcarl dongcarl force-pushed the 2018-12-integrate-bitcoin-hashes branch from c2bc4b1 to a3dc31b Compare January 24, 2019 16:32
@dongcarl
Copy link
Copy Markdown
Member Author

Fixed some minor feature naming so that Travis is happy.

sgeisler
sgeisler previously approved these changes Jan 24, 2019
Copy link
Copy Markdown
Contributor

@sgeisler sgeisler left a comment

Choose a reason for hiding this comment

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

Looked through all your commits, that must have been quite a bit of work, thank you :)

I think we should all revisit our own PRs after this gets merged, since many of them will need rebasing imo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can replace the bitcoin_hashes::Hash with bitcoin_hashes::sha256d::Hash::from_slice here to make it match the old version.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed!

Also replace unsafe transmute with call to read_u64_into
Not needed anymore as the bitcoin_hashes crate handles this.
We no longer need rust-crypto after integrating bitcoin_hashes.
@dongcarl dongcarl force-pushed the 2018-12-integrate-bitcoin-hashes branch from f2f3617 to ca72a04 Compare January 24, 2019 21:27
@tamasblummer
Copy link
Copy Markdown
Contributor

This is rather disruptive downstream. Maybe we should add an implementation of Sha256dHash that wraps sha256d::Hash and implements the most used methods, that are in my projects: Sha256dHash::default ::from ::from_data ::from_hex ::as_bytes

@TheBlueMatt
Copy link
Copy Markdown
Member

I think we generally do a major version bump almost-by-itself for a dep bump like this so that people can update at their leisure. If you need it downstream we could maybe land #218 and a few others in their own version first, but I'd rather not add wrappers to be removed in the next version :/.

@dongcarl
Copy link
Copy Markdown
Member Author

@sgeisler Please re-ACK, thanks!

@dongcarl
Copy link
Copy Markdown
Member Author

@tamasblummer I agree with @TheBlueMatt, I'm happy to rebase over your PRs if we wanna do a version with those first! 😄

@tamasblummer
Copy link
Copy Markdown
Contributor

I already use them as patches on 0.15. Feel free to merge this. I will rebase my PRs

@dongcarl dongcarl merged commit 51aba8b into rust-bitcoin:master Jan 25, 2019
tamasblummer added a commit to tamasblummer/rust-bitcoin that referenced this pull request Jan 25, 2019
tamasblummer added a commit that referenced this pull request Feb 8, 2019
* add BIP57 (Client Side Block Filtering) Messages

* rabased after #215
sidhujag pushed a commit to syscoin/rust-bitcoin that referenced this pull request Feb 9, 2019
* add BIP57 (Client Side Block Filtering) Messages

* rabased after rust-bitcoin#215
kyluke pushed a commit to kyluke/rust-bitcoin that referenced this pull request Jul 10, 2019
newtype implementation of opcodes::All

Removes unsafety when converting u8 -> All

safe implementation of All -> Ordinary

squashme: work around lack of associated constants

make opcode PR work with 1.14.0

add some opcode tests

Fix indentation in opcodes.rs

Fix comment on transaction version

Internalize unnecessarily exported macros

Bump secp to 0.12

Replace slow hex decoding function with optimized version

Fixes rust-bitcoin#207.

Remove unused Pair iterator and util::iter module

Add feature gated hex decode benchmark

bump version to 0.16

Bump rustc version to 1.22.0

it is annoying to have a difference between debug and print for hash

Fix typos

Run cargo bench on rustc nightly in travis, remote useless move

Add bitcoin_hashes dependency, rename some features

Because features and dependencies share the same namespace, and we want
to pass down the optional dependence on serde to bitcoin_hashes, we need
to rename the feature to something other than serde. Right now only
features can be passed down to dependencies.

Note that we could have also renamed the dependency to something like
serde-dep and kept the same feature name, however, dependency renaming
has only been available since cargo 0.27.0

Features that represent optional dependencies have been prefixed with
'use-'. The travis file has also been modified to conform to this
change.

Implement En/Decodable for sha256d::Hash

Convert codebase from util::hash to bitcoin_hashes

Also replace unsafe transmute with call to read_u64_into

Remove code deprecated by bitcoin_hashes from util::hash

Remove unused internal macro

Remove fuzz_util module

Not needed anymore as the bitcoin_hashes crate handles this.

Remove rust-crypto dependency

We no longer need rust-crypto after integrating bitcoin_hashes.

Fix typos and clarify some comment in blockdata, block, address (rust-bitcoin#230)

Remove Address::p2pk

There is no address format for p2pk.

add BIP157 (Client Side Block Filtering) Messages (rust-bitcoin#225)

* add BIP57 (Client Side Block Filtering) Messages

* rabased after rust-bitcoin#215

Cleanup util::privkey in preparation for PublicKey

- Rename privkey::PrivKey to privkey::PrivateKey
- Remove unnecessary methods for privkey::PrivateKey
- Modify tests to work with above changes

Add PublicKey struct encapsulating compressedness

- Move util::privkey to util::key
- Add PublicKey struct to util::key
- Implement de/serialization methods for util::key::PublicKey

Integrate newly-added PublicKey with Address

- Switch util::address::Payload::Pubkey variant to wrap
  util::key::PublicKey
- Switch util::address::Address::p*k* constructors to use
  util::key::PublicKey
- Fix tests for aforementioned switch
- Add convenience methods for util::key::PublicKey to
  util::key::PrivateKey conversion
- Switch BIP143 tests to use util::key::PublicKey

key: Reword and clarify comments

key: Use correct error for decoding

This change also moves the secp256k1::Error wrapper from util::Error to
consensus::encode::Error, since we do not use it anywhere else. We can
add it back to util::Error once we have instances of secp256k1::Error
that are not related to consensus::encode.

Extract travis testing into locally-runnable script

Extract the Script assembly creator from fmt::Debug

Added contributing part to README

point to IRC

bip32: ChildNumber constructors return Result

They can produce an error if the index is out of range.

bip32: Introduce DerivationPath type

Implements Display and FromStr for easy usage with serialized types.

bip32: Change test vectors to use DerivationPath

bip32: Add additional methods and traits to DerivationPath

- From<&[ChildNumber]> (cloning)
- AsRef<[ChildNumber]>
- std::iter::FromIterator<ChildNumber>
- std::iter::IntoIterator<ChildNumber>
- std::ops::Index (returning &[ChildNumber])

Also add two methods:
- child(&self, ChildNumber) -> DerivationPath
- into_child(self, ChildNumber) -> DerivationPath

Implement Witness commitment check for Block. Remove MerkleRoot implementations for types implementing BitcoinHash as
it is misleading. MerkleRoot is defined instead for a Block.

Forbid unsafe code

Remove extraneous clones in consensus::params

Remove unused Option en/decoding

Better RawNewtorkMessage deserealization from IO stream (rust-bitcoin#231)

Follow-up to rust-bitcoin#229

While working with remote peers over the network it is required to deserealize RawNetworkMessage from `TCPStream` to read the incoming messages. These messages can be partial – or one TCP packet can contain few of them. To make the library usable for such use cases, I have implemented the required functionality and covered it with unit tests.

Sample usage:
```rust
fn run() -> Result<(), Error> {
    // Opening stream to the remote bitcoind peer
    let mut stream = TcpStream::connect(SocketAddr::from(([37, 187, 0, 47], 8333));
    let start = SystemTime::now();

    // Constructing and sending `version` message to get some messages back from the remote peer
    let since_the_epoch = start.duration_since(UNIX_EPOCH)
        .expect("Time went backwards");
    let version_msg = message::RawNetworkMessage {
        magic: constants::Network::Bitcoin.magic(),
        payload: message::NetworkMessage::Version(message_network::VersionMessage::new(
            0,
            since_the_epoch.as_secs() as i64,
            address::Address::new(receiver, 0),
            address::Address::new(receiver, 0),
            0,
            String::from("macx0r"),
            0
        ))
    };
    stream.write(encode::serialize(&version_msg).as_slice())?;

    // Receiving incoming messages
    let mut buffer = vec![];
    loop {
        let result = StreamReader::new(&mut stream, None).read_messages();
        if let Err(err) = result {
            stream.shutdown(Shutdown::Both)?;
            return Err(Error::DataError(err))
        }
        for msg in result.unwrap() {
            println!("Received message: {:?}", msg.payload);
        }
    }
}
```

Sample output is the following:
```
Received message: Version(VersionMessage { version: 70015, services: 1037, timestamp: 1548637162, receiver: Address {services: 0, address: [0, 0, 0, 0, 0, 65535, 23536, 35968], port: 33716}, sender: Address {services: 1037, address: [0, 0, 0, 0, 0, 0, 0, 0], port: 0}, nonce: 1370726880972892633, user_agent: "/Satoshi:0.17.99/", start_height: 560412, relay: true })
Received message: Verack
Received message: Alert([1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 127, 0, 0, 0, 0, 255, 255, 255, 127, 254, 255, 255, 127, 1, 255, 255, 255, 127, 0, 0, 0, 0, 255, 255, 255, 127, 0, 255, 255, 255, 127, 0, 47, 85, 82, 71, 69, 78, 84, 58, 32, 65, 108, 101, 114, 116, 32, 107, 101, 121, 32, 99, 111, 109, 112, 114, 111, 109, 105, 115, 101, 100, 44, 32, 117, 112, 103, 114, 97, 100, 101, 32, 114, 101, 113, 117, 105, 114, 101, 100, 0])
```

Working sample code can be found here: https://github.com/dr-orlovsky/bitcoinbigdata-netlistener

key: add some missing functionality

bip32: replace rust-secp key types with rust-bitcoin key types

We continue to support only compressed keys when doing key derivation,
but de/serialization of uncompressed keys will now work, and it will
be easier/more consistent to implement PSBT on top of this.

Add PSBT-specific Error data type

- Implement psbt::Error data type
- Implement conversion from psbt::Error to util::Error
- Create util::psbt module
- Create non-public util::psbt::error module

Add data types for raw PSBT key-value pairs

- Add (en)decoding logic for said data types

Add trait for PSBT key-value maps

Add PSBT global data key-value map type

- Implement psbt::Map trait for psbt::Global
- Add converting constructor logic from Transaction for psbt::Global
- Add (en)decoding logic for psbt::Global
  - Always deserialize unsigned_tx as non-witness

- Add trait for PSBT (de)serialization
- Implement PSBT (de)serialization trait for relevant psbt::Global types

- Add macros for consensus::encode-backed PSBT (de)serialization
  implementations
- Add macro for implementing encoding logic for PSBT key-value maps

Add PSBT output data key-value map type

- Implement psbt::Map trait for psbt::Output
- Add (en)decoding logic for psbt::Output

- Implement PSBT (de)serialization trait for relevant psbt::Output types

- Add macro for merging fields for PSBT key-value maps
- Add macro for implementing decoding logic for PSBT key-value maps
- Add convenience macro for implementing both encoding and decoding
  logic for PSBT key-value maps
- Add macro for inserting raw PSBT key-value pairs into PSBT key-value
  maps
- Add macro for getting raw PSBT key-value pairs from PSBT key-value
  maps

Add PSBT input data key-value map type

- Implement psbt::Map trait for psbt::Input
- Add (en)decoding logic for psbt::Input

- Implement PSBT (de)serialization trait for relevant psbt::Input types

Add Partially Signed Transaction type

- Add merging logic for PartiallySignedTransactions
- Add (en)decoding logic for PartiallySignedTransaction
- Add converting constructor logic from Transaction for
  PartiallySignedTransaction
- Add extracting constructor logic from PartiallySignedTransaction for
  Transaction

Squashed in fixes from stevenroose <stevenroose@gmail.com>

- Prevent PSBT::extract_tx from panicking
- Make PartiallySignedTransaction fields public

Add test vectors from BIP174 specification

- Add macro for decoding and unwrapping PartiallySignedTransaction from
  hex string

Add fuzz testing for PartiallySignedTransaction

Add copyright notice to PSBT-related files

bump version to 0.17.0

add rust-bitcoin#231 to CHANGELOG for 0.17.0

key: implement ToString and FromStr for PublicKey

script: add `push_key` function to Builder to allow serializing public keys more easily

bump version to 0.17.1

Bump bitcoin-bech32 dependency

This makes the Address::Payload::WitnessProgram inner type compatible
with rust-lightning-invoice's Fallback::SegWitProgram's inner type.
This allows specifying fallbacks from addresses.

util::key: Provide to_bytes() methods for key types

These are mainly utility methods around the existing way to serialize
the key types.

util::key add serde de/serialization

contracthash: add fixed test vector

contracthash: use `PublicKey` and `PrivateKey` types; minor cleanups

contracthash: more cleanups

bump version to 0.18

Fix nit in CHANGELOG.md

bip32: Add DerivationPathIterator and related methods

Adds methods
- ChildNumber::increment
- DerivationPath::children_from
- DerivationPath::normal_children
- DerivationPath::hardened_children

Drop LoneHeaders and just use BlockHeader

The protocol has a bug where a 0u8 is pushed at the end of each
block header on the wire in headers messages. WHy this bug came
about is unrealted and shouldn't impact API design.

Implement util::misc::signed_msg_hash()

Add slice consensus encode/decode functions and use for short arrays

Swap a few more [d]encoders to slice emit/read functions

Support sendheaders network message decode

Drop some unused/not-needed Encodable impls

Speed up Vec<u8> [d]e[n]code operations by dropping the generic

Decrease travis-fuzz iterations to fix hangs

Two serde quirks from switching dependencies

Add Amount and SignedAmount types

Add fuzz target for Amount parsing

Fix trivial DoS when deserializing messages from the network

fuzz: Add fuzzer for RawNetworkMessage.

Rename deserialize_raw_network_message to make my afl scripts happy

Switch Travis fuzzing to 30 seconds per target from an iter count.

Rename BlockHeader::spv_validate to validate_pow

Remove confusing mentions of SPV

Rename OP_NOP2 and OP_NOP3 to OP_CLTV and OP_CSV

Add OutPoint::new() for one-liner construction (rust-bitcoin#285)

Remove Decimal and replace strason with serde_json

Slightly update README
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants