Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Address::p2pk #222

Merged
merged 1 commit into from
Feb 7, 2019
Merged

Remove Address::p2pk #222

merged 1 commit into from
Feb 7, 2019

Conversation

stevenroose
Copy link
Collaborator

@stevenroose stevenroose commented Jan 21, 2019 •
edited
Loading

There is no address format for p2pk.

Script Descriptors should probably be used to construct scriptPubkeys and scriptSigs for using them instead of the Address type.

@stevenroose stevenroose mentioned this pull request Jan 21, 2019
Closed
@tamasblummer
Copy link
Contributor

Pay to public key is used in Bitcoin blockchain, with most of satoshi's coins. Why should it be removed?

@stevenroose
Copy link
Collaborator Author

@tamasblummer Because it's not an address. It's just a script. Script descriptors make it possible to construct scriptPubKeys for them, so we don't need to have them awkwardly put in the Address type.

@tamasblummer
Copy link
Contributor

This script is usually interpreted by explorer as sending to an address. I do not see value in a PR removing an interpretation that is common.

@stevenroose
Copy link
Collaborator Author

Yeah, that's a wrong interpretation of that script. And more modern explorers will also not show it as such.

I agree that it's unfortunate to remove features from a codebase. But it's in the wrong place and script descriptors can cover the lost functionality.

@dongcarl
Copy link
Member

Hmmm I remember @apoelstra had some thoughts on this.

@apoelstra
Copy link
Member

Yeah, fine by me. If the ambiguous interpretation is not even universally used I'd rather drop it because it's confusing and could lead to lost coins.

@dongcarl
Copy link
Member

dongcarl commented Feb 7, 2019

Sounds good. @stevenroose please rebase!

@stevenroose
Copy link
Collaborator Author

@dongcarl done.

dongcarl
dongcarl approved these changes Feb 7, 2019
View reviewed changes
Copy link
Member

@dongcarl dongcarl left a comment

Choose a reason for hiding this comment

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

utACK f80e882

@stevenroose
Remove Address::p2pk
f80e882 
There is no address format for p2pk.
sgeisler
sgeisler approved these changes Feb 7, 2019
View reviewed changes
Copy link
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.

ACK since it gets the Address type closer to always supporting round trips (I don't know if it's the case yet and think we should add fuzz tests for it at some point).

@dongcarl dongcarl merged commit e386d9e into rust-bitcoin:master Feb 7, 2019
dr-orlovsky added a commit to datagnition/rust-bitcoin that referenced this pull request Feb 9, 2019
@dr-orlovsky
Squashed commit of the following:
c960679 
commit c727897
Merge: b8bd1d9 1cd2782
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sat Feb 9 13:44:12 2019 +0200

    Merge branch 'master' into network-streaming

commit 1cd2782
Author: Tamás Blummer <tamas.blummer@gmail.com>
Date:   Fri Feb 8 13:00:51 2019 +0100

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

    * add BIP57 (Client Side Block Filtering) Messages

    * rabased after rust-bitcoin#215

commit e386d9e
Merge: 51971dd f80e882
Author: Carl Dong <accounts@carldong.me>
Date:   Thu Feb 7 16:18:31 2019 -0500

    Merge pull request rust-bitcoin#222 from stevenroose/no-p2pk-addr

    Remove Address::p2pk

commit f80e882
Author: Steven Roose <steven@stevenroose.org>
Date:   Mon Jan 21 16:40:04 2019 +0000

    Remove Address::p2pk

    There is no address format for p2pk.

commit b8bd1d9
Author: Tamas Blummer <tamas.blummer@gmail.com>
Date:   Thu Feb 7 03:37:49 2019 +0100

    return a single message

commit e82407b
Merge: f509fc1 51971dd
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Tue Feb 5 03:27:41 2019 +0200

    Merge branch 'master' of https://github.com/rust-bitcoin/rust-bitcoin

commit f509fc1
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Tue Feb 5 03:27:10 2019 +0200

    Verbose comments explaining the workflow for stream parsing in `StreamReader`

commit 51971dd
Author: ariard <ariard@student.42.fr>
Date:   Mon Feb 4 07:30:41 2019 +0100

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

commit ff85da8
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Feb 4 03:37:29 2019 +0200

    Fixed tests failure in StreamReader

    caused by parallel-runned tests opening two simultaneuos TCP sockets on the same port

commit 50bb68d
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Feb 4 02:31:58 2019 +0200

    Additional complex unit test for StreamReader from real-world case of Bitcoin Core communications

    grabbed with Wireshark

commit d590101
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Feb 4 02:03:56 2019 +0200

    Improved & more detailed unit tests for StreamReader

    using both file & TcpStream IO, covering private methods

commit e68c186
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Feb 3 23:04:14 2019 +0200

    Making tests passing

commit 4ed9fd8
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Feb 3 22:31:06 2019 +0200

    Initial unit tests for new StreamReader

commit 150f00a
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Feb 3 22:29:57 2019 +0200

    Fixing compuler warning on the order of trait methods implementation

commit 27f8ed7
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Feb 3 22:29:30 2019 +0200

    Moving StreamReader to a separate file and abstracting its functionality out of RawNetworkMessage scope

    as suggested at rust-bitcoin#231 (comment)

commit cd1a170
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Feb 3 18:38:13 2019 +0200

    Fixing rust-bitcoin#231 (comment)

commit 48021db
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sat Feb 2 21:17:53 2019 +0200

    Configuring network::message::RawNetworkMessage.from_stream buffer size and iterations with special struct

commit 4426a14
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sat Feb 2 20:42:33 2019 +0200

    Improved consensus.encode.deserealize_partial function signature

    Now function communicateds back to the caller the amount of consumed data by adding Cursor part to the Result value tuple – instead of using &mut argument.
    This have led to simplified and more straightforward logic of the function and its callers.

commit f5587cd
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 03:23:22 2019 +0200

    Documented `deserialize_partial` function

commit 4d464fa
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 02:59:02 2019 +0200

    Fixed error reporting on incomplete message deserealization from a stream

commit 365facc
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 02:35:57 2019 +0200

    Stream reading unit test updated with more complex case

commit 34a00e8
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 02:35:34 2019 +0200

    Improved stream reading algorithm

commit 4410af8
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 01:54:25 2019 +0200

    Stream deserealization unit test case

commit d3afdc4
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 01:54:02 2019 +0200

    Generalizing stream deserialization (from TcpStream to any IO stream)

commit 28a1ffa
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 01:29:38 2019 +0200

    Improving buffering while deserializing from TcpStream

commit e382886
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 01:23:52 2019 +0200

    Trying reading TcpStream with BufRead

commit 6abbd15
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Mon Jan 28 00:31:12 2019 +0200

    Hiding network::Error conversion trait from docs

commit 940d63d
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Jan 27 23:52:16 2019 +0200

    Support for automatic IO errors in ? syntax

commit afcc7c8
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Jan 27 23:10:05 2019 +0200

    Function that parses TcpStream and creates RawNetworkMessage out of it: `RawNetworkMessage::from_tcpstream`

commit e6ee7e5
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Jan 27 19:44:12 2019 +0200

    Improved unit tests for message peyload deserialization

commit 83acf9a
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Jan 27 19:39:03 2019 +0200

    Desearilizing partial message content, which will be required lately for reading TcpStream partial content

commit 4a16bcc
Author: Dr. Maxim Orlovsky <dr.orlovsky@gmail.com>
Date:   Sun Jan 27 19:37:49 2019 +0200

    Support for debug mode formattin in RawNetworkMessage
casey pushed a commit to casey/rust-bitcoin that referenced this pull request Nov 28, 2023
@stevenroose
Merge pull request rust-bitcoin#222 from tcharding/05-18-edition-2018
c861370 
Enable edition 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgeisler sgeisler sgeisler approved these changes

@dongcarl dongcarl dongcarl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@stevenroose @tamasblummer @dongcarl @apoelstra @sgeisler