Make network and rpcport configurable rather than hardcoding them

[jump-kick]

Fixes #56.

• Add network and rpcport fields to properties class
• Add validation for the new fields
• Add unit tests

[theborakompanioni]

Looks good!

I think the p2p port should also be exposed right?
At least it was in the (now commented out) section:

List<Integer> hardcodedStandardPorts = ImmutableList.<Integer>builder()
        .addAll(this.properties.getChain() == Chain.mainnet ? Lists.newArrayList(8332, 8333) : Collections.emptyList())
        .addAll(this.properties.getChain() == Chain.testnet ? Lists.newArrayList(18332, 18333) : Collections.emptyList())
        .addAll(this.properties.getChain() == Chain.regtest ? Lists.newArrayList(18443, 18444) : Collections.emptyList())
        .build();

Also, if rpcport is not specified, we should use the default ports of the specific network (regtest is the default):

• mainnet: 8332 (rpc), 8333 (p2p)
• testnet: 18332 (rpc), 18333 (p2p)
• regtest: 18443 (rpc), 18444 (p2p)

What do you think?

[jump-kick]

Thanks for the feedback!
Ah, ok, I missed the P2P port. I did add in defaults in the Properties class for RPC port and Network like this:
private String network = DEFAULT_CHAIN.toString();
private Integer rpcport = REGTEST_DEFAULT_RPC_PORT;

And then those defaults are effectively used in setting the exposed ports for the testcontainer:
List<Integer> exposedPorts = ImmutableList.<Integer>builder() .add(this.properties.getRpcport() .get()) .addAll(this.properties.getExposedPorts()) .build();

I could add a P2P port property to the properties class:
private Integer p2pport;

But I think you are also saying you'd like to be able to have logic to determine what the default ports are based on Network? If Regtest (default), then 18443 and 18444. If mainnet, then 8332 and 8333, etc. Is that right? If so, I can certainly do that.

Also, I'm assuming in the case RPC port is provided, but P2P port is blank, I should use the default P2P port for the network.

Then I would probably add a PostConstruct method to check what the Network is and set the ports based on that. But the default would be Regtest.

And then add those in the exposedPorts List:
List<Integer> exposedPorts = ImmutableList.<Integer>builder() .add(this.properties.getRpcport() .add(this.properties.getP2pport()) .addAll(this.properties.getExposedPorts()) .build();

What do you think?

[theborakompanioni]

Thanks for the feedback! Ah, ok, I missed the P2P port. I did add in defaults in the Properties class for RPC port and Network like this: private String network = DEFAULT_CHAIN.toString(); private Integer rpcport = REGTEST_DEFAULT_RPC_PORT;

I could add a P2P port property to the properties class: private Integer p2pport;

👍

But I think you are also saying you'd like to be able to have logic to determine what the default ports are based on Network? If Regtest (default), then 18443 and 18444. If mainnet, then 8332 and 8333, etc. Is that right? If so, I can certainly do that.

Yes.

Also, I'm assuming in the case RPC port is provided, but P2P port is blank, I should use the default P2P port for the network.

That is what I was thinking, yes. What do you think?

Then I would probably add a PostConstruct method to check what the Network is and set the ports based on that. But the default would be Regtest.

Currently, most default values of classes with @ConfigurationProperties have their default values applied via getters.
If you want, take a look at ClnContainerProperties, it does already something similar.

I like that more than PostConstruct as it is simple and not using spring automagic. But there are also downsides - what do you prefer?

And then add those in the exposedPorts List: List<Integer> exposedPorts = ImmutableList.<Integer>builder() .add(this.properties.getRpcport() .add(this.properties.getP2pport()) .addAll(this.properties.getExposedPorts()) .build();

What do you think?

Nice.

If network, rpcport and p2pport all return non-null values only, they don't need to return Optional anymore 💪

[jump-kick]

Update: I am working this, and sorry for the delay! I'm so close to having it done, but am now having trouble with the unit tests in testcontainer. When running a test with mainnet or testnet configuration, the container times out:

org.testcontainers.containers.ContainerLaunchException: Timed out waiting for container port to open (127.0.0.1 ports: [56180] should be listening

Not sure in this case where port 56180 is coming from, since I am setting 8332 and 8333 for Mainnet. I'm new to Testcontainers, so I might be missing something. Will keep you posted

[theborakompanioni]

Update: I am working this, and sorry for the delay! I'm so close to having it done, but am now having trouble with the unit tests in testcontainer. When running a test with mainnet or testnet configuration, the container times out:

org.testcontainers.containers.ContainerLaunchException: Timed out waiting for container port to open (127.0.0.1 ports: [56180] should be listening

Not sure in this case where port 56180 is coming from, since I am setting 8332 and 8333 for Mainnet. I'm new to Testcontainers, so I might be missing something. Will keep you posted

Hey @jump-kick, take your time! No urgency involved.
Can you tell which test (which module) is failing? Some of them (e.g. electrum testcontainers) can be a bit flaky - thinking about externalizing (move to other repo9 or removing them in the future, as I do not think they are used by anyone.

Do not be afraid of pushing your changes - I'd love to review them 🙌

[jump-kick]

Ha, thanks for your patience! I just pushed another commit for your review. The failing tests were two new ones I added, 'defaultMainnetValuesTest' and 'defaultTestnetValuesTest' where I set the network to 'mainnet' and 'testnet'. THEN, I noticed the comment in BitcoindTestcontainerAutoConfiguration that says "NOTE: Currently only really supports creating "regtest" container. Defaults do not play nicely with mainnet/testnet -> "networkactive" is disabled." That seemed to be my experience, too, so I tried a way to do the unit tests where the testcontainer is not started; although maybe that defeats the purpose of this project, haha. oh well. See 'defaultMainnetValuesTest' and 'defaultTestnetValuesTest'.

Now I don't have the testcontainer problem where it times out waiting for a port to open, but there is some weird bug where the 'network' property is not being set, and it keeps returning the default 'regtest'. I'm sure it's something obvious and I will kick myself when I realize the problem = ).

[jump-kick]

oh yeah, and I like your idea of putting the default logic in the getters, so I went ahead with that.

[theborakompanioni]

@jump-kick So nice!

I think I will have time to review it next week. Is it okay to push some changes (e.g. remove logging in property class) or do you favor that I describe them here and you "fix" them yourself? Thank you so much 🙏

[jump-kick]

Yes, please feel free to push any changes you have! I fixed a thing or two after pushing this, but still having unit test issues. Hope to push some changes myself tomorrow, and I can remove that logging if you don't get to it first.

…

On Sat, May 4, 2024, 8:09 AM Thebora Kompanioni ***@***.***> wrote: @jump-kick <https://github.com/jump-kick> So nice! I think I will have time to review it next week. Is it okay to push some changes (e.g. remove logging in property class) or do you favor that I describe them here and you "fix" them yourself? Thank you so much 🙏 — Reply to this email directly, view it on GitHub <#126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGUKH7OJJMLN2HW4GN5763DZATFWTAVCNFSM6AAAAABGIMEE5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJUGE2DCNZQHA> . You are receiving this because you were mentioned.Message ID: ***@***.*** .com>

[theborakompanioni]

Yes, please feel free to push any changes you have!

PR against your branch: jump-kick#1
Merge, if you think these adaptions are appropriate 🙏

[theborakompanioni]

@jump-kick What do you think.. should p2pport be renamed to port and network to chain?

[jump-kick]

Yes, I like that idea! I was thinking I could implement those changes and then add them to the branch/PR and make it official. I could have that done within a couple of days.

…

On Tue, May 28, 2024 at 4:24 AM Thebora Kompanioni ***@***.***> wrote: @jump-kick <https://github.com/jump-kick> What do you think.. should p2pport be renamed to port and network to chain? — Reply to this email directly, view it on GitHub <#126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGUKH7PGVLROBK6OBVM4ZWLZEQ5MVAVCNFSM6AAAAABGIMEE5SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZUGYZDOMBUGM> . You are receiving this because you were mentioned.Message ID: ***@***.*** .com>

[theborakompanioni]

Yes, I like that idea! I was thinking I could implement those changes and then add them to the branch/PR and make it official. I could have that done within a couple of days.

Nice! Take your time - there is no urgency involved. Looking forward to your contribution 🧡

[jump-kick]

One quick question, @theborakompanioni - can you confirm this is right?

bitcoin-spring-boot-starter/spring-testcontainer/spring-testcontainer-bitcoind-starter/src/main/java/org/tbk/spring/testcontainer/bitcoind/config/BitcoindContainerProperties.java

Line 73 in 3a2c2f7

mainnet("main"),

bitcoin-spring-boot-starter/spring-testcontainer/spring-testcontainer-bitcoind-starter/src/main/java/org/tbk/spring/testcontainer/bitcoind/config/BitcoindContainerProperties.java

Line 74 in 3a2c2f7

testnet("test"),

"main" vs. "mainnet" and "test" vs. "testnet" for the Enum String property?

[theborakompanioni]

One quick question, @theborakompanioni - can you confirm this is right?

bitcoin-spring-boot-starter/spring-testcontainer/spring-testcontainer-bitcoind-starter/src/main/java/org/tbk/spring/testcontainer/bitcoind/config/BitcoindContainerProperties.java

Line 73 in 3a2c2f7

mainnet("main"),

bitcoin-spring-boot-starter/spring-testcontainer/spring-testcontainer-bitcoind-starter/src/main/java/org/tbk/spring/testcontainer/bitcoind/config/BitcoindContainerProperties.java

Line 74 in 3a2c2f7

testnet("test"),

"main" vs. "mainnet" and "test" vs. "testnet" for the Enum String property?

Yeah, you are right - consistent would be to make them the same values as Bitcoin Core. However, they are named like they are now through-out other modules as well. I'd say let's keep it like it is and maybe think about a rectification of all properties in a follow-up ticket/PR. Okay for you?

I think this should be merged soon.

[jump-kick]

Sounds good to me! Thanks

[theborakompanioni]

Sounds good to me! Thanks

Nice. Will merge this now into devel. There are still some failing tests and upon inspection, I will rename chain to network again - as it really is different from the Bitcoin Core values ("main" vs "mainnet", etc.). Fixing this in a follow-up PR!

Thank you so much for your contribution @jump-kick 🚀 🚀 🚀

[theborakompanioni]

Daaamn. Did not look at the target branch and merged into master by mistake. Will fix this as soon as I am back on my workstation. 😬

[theborakompanioni]

Daaamn. Did not look at the target branch and merged into master by mistake. Will fix this as soon as I am back on my workstation. 😬

Fixed 🙏

[jump-kick]

d'oh! Sorry if I put the wrong branch there! Thanks!!