Skip to content

Conversation

@fiatjaf
Copy link

@fiatjaf fiatjaf commented Apr 23, 2022

  • remove useless SelectParams call since bitcoinlib doesn't know signet.
  • remove generateblocks, replace it with a message telling the user to get signet coins.
  • hardcode demo scenario to signet.
  • output the "txid:output" as the original_coin instead of just the txid.
  • update readme.

Copy link
Owner

@jamesob jamesob left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Before something like this is merged, we need to figure out how to preserve optional regtest use. I'll propose a patch set that does this (with you as co-author) unless you get around to it first.

README.md Outdated

This repository demonstrates an implementation of simple, "single-hop" vaults
using the proposed `OP_CHECKTEMPLATEVERIFY` opcode.
using the proposed `OP_CHECKTEMPLATEVERIFY` opcode.
Copy link
Owner

Choose a reason for hiding this comment

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

Would've preferred these doc/whitespace changes in a separate commit, but no big deal.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, my editor did this and I know it sucks, but I thought it wasn't a very big deal.

I can rebase and remove these though.

# https://github.com/JeremyRubin/bitcoin/tree/checktemplateverify-rebase-4-15-21
$ bitcoind -regtest -txindex=1 &
# https://github.com/JeremyRubin/bitcoin/tree/checktemplateverify-signet-23.0-alpha
$ bitcoind -signet -signetchallenge=512102946e8ba8eca597194e7ed90377d9bbebc5d17a9609ab3e35e706612ee882759351ae -signetseednode=50.18.75.225 -txindex=1 &
Copy link
Owner

Choose a reason for hiding this comment

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

General feedback on this PR: we need to both support signet and regtest, so it would be useful to segment the various parts of this change out and keep regtest-specific content intact.

.get_private_key(1)
.point.p2wpkh_address(network=rpc.net_name)
)
return rpc.generatetoaddress(n, addr)
Copy link
Owner

Choose a reason for hiding this comment

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

In line with preserving regtest, let's keep this.


@classmethod
def from_network(cls, network: str, seed: bytes, coin: Coin = None, **plan_kwargs):
SelectParams(network)
Copy link
Owner

Choose a reason for hiding this comment

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

Note to self to verify that this doesn't actually do anything

c = VaultScenario.from_network(
"regtest", seed=b"demo", coin=coin_in, block_delay=10
)
c = VaultScenario.from_network("signet", b"demo", coin=coin_in, block_delay=10)
Copy link
Owner

Choose a reason for hiding this comment

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

Hardcoded network constants should be factored out into some kind of SelectParams-ish global behavior that still allows use of regtest.

Copy link
Author

Choose a reason for hiding this comment

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

SelectParams wasn't being used for anything as far as I know, and you already had a global registry of ports for each network.

main.py Outdated
def _broadcast_final(c: VaultScenario, tx: CTransaction, hot_or_cold: str):
print()
if hot_or_cold == 'cold':
if hot_or_cold == "cold":
Copy link
Owner

Choose a reason for hiding this comment

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

Would be good to separate out formatting changes if possible.



def no_output(*args, **kwargs):
print(*args, file=sys.stderr, **kwargs)
Copy link
Owner

Choose a reason for hiding this comment

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

We need to revisit this since it definitely isn't well named as "no_output."

Copy link
Author

Choose a reason for hiding this comment

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

That was the name it had, I was very confused by it 😛

fiatjaf added 3 commits April 24, 2022 15:32
- remove useless SelectParams call since bitcoinlib doesn't know signet.
- remove generateblocks, replace it with a message telling the user to get signet coins.
- hardcode demo scenario to signet.
- output the "txid:output" as the original_coin instead of just the txid.
@fiatjaf
Copy link
Author

fiatjaf commented Apr 24, 2022

I've removed the whitespace and quote changes and reduced the README changes to a minimum and brought multi-network support back, although the demo still defaults to signet and there is no way to change it. I think there could be a global flag for specifying the network but I'm not sure how to do it, so I'll leave that to you.

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.

2 participants