-
Notifications
You must be signed in to change notification settings - Fork 633
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
Conversation
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. |
9b00e4b Fixing bug where to_bytes clobbers testnet params. (Ethan Heilman)
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. But what @EthanHeilman demonstrates is clearly an bug. How about instead of removing the |
@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. |
Ahh, good point. How about using the def foo(params=None):
if params is None:
params = bitcoin.params
… approach then? |
@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. |
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? |
I'd like to point out that there are only three occurrences of a $ 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 |
9b00e4b Fixing bug where to_bytes clobbers testnet params. (Ethan Heilman) [ yapified by gitreformat (github/ghtdak) on Mon Nov 30 21:11:40 2015 ]
To replicate the bug run the following commands in python console:
Notice that the magic byte is set for main, not testnet.
After my change:
Notice that the magic byte(s) are correct for testnet.
Happy new years. =)