Skip to content

Fixing bug where to_bytes clobbers testnet params. - #38

Merged
petertodd merged 1 commit into
petertodd:masterfrom
EthanHeilman:master
Jan 1, 2015
Merged

Fixing bug where to_bytes clobbers testnet params.#38
petertodd merged 1 commit into
petertodd:masterfrom
EthanHeilman:master

Conversation

@EthanHeilman

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Ahh, good point. How about using the

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

approach then?

@petertodd

Copy link
Copy Markdown
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

rnicoll commented Jan 3, 2015

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

4 participants