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.

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

Fixing bug where to_bytes clobbers testnet params. #38

Merged
merged 1 commit into from
Jan 1, 2015

Conversation

EthanHeilman
Copy link
Contributor

To replicate the bug run the following commands in python console:

import bitcoin
from bitcoin.messages import msg_addr
bitcoin.SelectParams('testnet')
msg_addr().to_bytes()[0]
'\xf9'

Notice that the magic byte is set for main, not testnet.

After my change:

import bitcoin
from bitcoin.messages import msg_addr
bitcoin.SelectParams('testnet')
msg_addr().to_bytes()[0]
'\x0b'

Notice that the magic byte(s) are correct for testnet.

Happy new years. =)

@petertodd
Copy link
Owner

Good catch! That's actually @Flowdalic's code, but the practice of not having per-function "params" methods is something I'm inclined to keep to keep the philosophy of testnet being as similar to mainnet as possible to ensure testnet is a valid simulation environment.

So I'm going to merge this, though I'm not going to rush to do a release just for this to give people time to (again) object.

@petertodd petertodd merged commit 9b00e4b into petertodd:master Jan 1, 2015
petertodd added a commit that referenced this pull request Jan 1, 2015
9b00e4b Fixing bug where to_bytes clobbers testnet params. (Ethan Heilman)
@Flowdalic
Copy link
Contributor

When I started using python-bitcoinlib for my project I thought I would maybe necessary to use the same python process in different networks (testnet, mainnet). I actually never needed this, but still could see that I may be good to have in certain situations. params as parameter with a default argument also existed in python-bitcoinlib before
0e4e1b2

But what @EthanHeilman demonstrates is clearly an bug. How about instead of removing the params argument, switching its default from MainParams() to bitcoin.params (If that's possible)?

@EthanHeilman
Copy link
Contributor Author

"How about instead of removing the params argument, switching its default from MainParams() to bitcoin.params"

@Flowdalic If it works that seems like the best option, note through that default arguments are determined when the function is defined, not when the function is called.
http://docs.python-guide.org/en/latest/writing/gotchas/

@Flowdalic
Copy link
Contributor

Ahh, good point. How about using the

def foo(params=None):
  if params is None:
    params = bitcoin.params

approach then?

@petertodd
Copy link
Owner

@Flowdalic That's not unreasonable, though it would add some boilerplate to every parameter using function.

Regarding the default argument issue, remember that params itself can be a object whose contents are changed when the parameters are set. Given the parameters are a global - quite intentionally so - that may be a reasonable design.

However lets see what @rnicoll comes back with in pull-req #39 first.

@rnicoll
Copy link
Contributor

rnicoll commented Jan 3, 2015

I think the "params=None" option makes a lot of sense. I'd presume this would only apply to class constructors, and then be tracked within the object?

@Flowdalic
Copy link
Contributor

I'd like to point out that there are only three occurrences of a params parameter in python-bitcoinlib master with 9b00e4b reverted, and the one in rpc.py is not related (Right now, it appears I was wrong stating that a params parameter existed before my commit, sorry about that).

$ ag 'params='
bitcoin/messages.py
54:    def to_bytes(self, params=MainParams()):
77:    def stream_deserialize(cls, f, params=MainParams(), protover=PROTO_VERSION):

bitcoin/rpc.py
444:    def submitblock(self, block, params=None):

I've created #42 as suggested solution

ghtdak pushed a commit to ghtdak/python-bitcoinlib that referenced this pull request Dec 1, 2015
9b00e4b Fixing bug where to_bytes clobbers testnet params. (Ethan Heilman)

 [ yapified by gitreformat (github/ghtdak) on Mon Nov 30 21:11:40 2015 ]
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