Switch to private testnet and upgrade - #105
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
🟢 Coverage ∅ diff coverage · -0.24% coverage variation
Metric Results Coverage variation ✅ -0.24% coverage variation (-1.00%) Diff coverage ✅ ∅ diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (8e77d6c) 828 501 60.51% Head commit (b069001) 828 (+0) 499 (-2) 60.27% (-0.24%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#105) 0 0 ∅ (not applicable) Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
Might make more sense to use regtest edit: i just noticed that it was using regtest before. so, nack changing to a private testnet. All of this can be done more cleanly using a private regtest network. |
070a249 to
cab1802
Compare
| "43Pnj6ZKGFTJhaLhiecSFfLfr64KPJZw7MyGH73T6PTDekBBvsTAaWEUSM4bmJqDuYLizhA13jQkMRPpz9VXBCBqQQb6y5L", | ||
| viewkey = "1bfa03b0c78aa6bc8292cf160ec9875657d61e889c41d0ebe5c54fd3a2c4b40e", | ||
| "9yEzCbcYdg6MqZ5AkEh8V3YCriyN1tvmtWEHdBEUHkF6D6kN1MMD2Kd2QVWoTY67aNHNYKMUP3xfteLS2QNavJxpJdx6mWj", | ||
| viewkey = "1f4668e8c1979b4c7dae13dc149fd95cd7ff2883becffe160c21f9e02c821c08", |
There was a problem hiding this comment.
Why not use regtest and leave these values unchanged?
| "43Pnj6ZKGFTJhaLhiecSFfLfr64KPJZw7MyGH73T6PTDekBBvsTAaWEUSM4bmJqDuYLizhA13jQkMRPpz9VXBCBqQQb6y5L"); | ||
| "9yEzCbcYdg6MqZ5AkEh8V3YCriyN1tvmtWEHdBEUHkF6D6kN1MMD2Kd2QVWoTY67aNHNYKMUP3xfteLS2QNavJxpJdx6mWj"); | ||
| await s.Page.Locator("input#PrivateViewKey") | ||
| .FillAsync("1bfa03b0c78aa6bc8292cf160ec9875657d61e889c41d0ebe5c54fd3a2c4b40e"); | ||
| .FillAsync("1f4668e8c1979b4c7dae13dc149fd95cd7ff2883becffe160c21f9e02c821c08"); |
| .FillAsync("1bfa03b0c78aa6bc8292cf160ec9875657d61e889c41d0ebe5c54fd3a2c4b40e"); | ||
| .FillAsync("1f4668e8c1979b4c7dae13dc149fd95cd7ff2883becffe160c21f9e02c821c08"); |
| PrimaryAddress = "43Pnj6ZKGFTJhaLhiecSFfLfr64KPJZw7MyGH73T6PTDekBBvsTAaWEUSM4bmJqDuYLizhA13jQkMRPpz9VXBCBqQQb6y5L", | ||
| PrivateViewKey = "1bfa03b0c78aa6bc8292cf160ec9875657d61e889c41d0ebe5c54fd3a2c4b40e", | ||
| WalletFileName = "wallet", | ||
| Password = "" | ||
| }); | ||
| await moneroRpcProvider.WalletRpcClients["XMR"] | ||
| .SendCommandAsync<GenerateFromKeysRequest, GenerateFromKeysResponse>("generate_from_keys", | ||
| new GenerateFromKeysRequest | ||
| { | ||
| PrimaryAddress = | ||
| "9yEzCbcYdg6MqZ5AkEh8V3YCriyN1tvmtWEHdBEUHkF6D6kN1MMD2Kd2QVWoTY67aNHNYKMUP3xfteLS2QNavJxpJdx6mWj", | ||
| PrivateViewKey = "1f4668e8c1979b4c7dae13dc149fd95cd7ff2883becffe160c21f9e02c821c08", |
| .FillAsync("43Pnj6ZKGFTJhaLhiecSFfLfr64KPJZw7MyGH73T6PTDekBBvsTAaWEUSM4bmJqDuYLizhA13jQkMRPpz9VXBCBqQQb6y5L"); | ||
| .FillAsync("9yEzCbcYdg6MqZ5AkEh8V3YCriyN1tvmtWEHdBEUHkF6D6kN1MMD2Kd2QVWoTY67aNHNYKMUP3xfteLS2QNavJxpJdx6mWj"); | ||
| await s.Page.Locator("input#PrivateViewKey") | ||
| .FillAsync("1bfa03b0c78aa6bc8292cf160ec9875657d61e889c41d0ebe5c54fd3a2c4b40e"); | ||
| .FillAsync("1f4668e8c1979b4c7dae13dc149fd95cd7ff2883becffe160c21f9e02c821c08"); |
| container_name: node_1 | ||
| command: [ | ||
| "monerod", | ||
| "--fixed-difficulty=150", |
There was a problem hiding this comment.
I presume this is changed from 1 as to limit the block rate?
There was a problem hiding this comment.
this a battle tested value from haveno-ts https://github.com/haveno-dex/haveno-ts/blob/master/docker/docker-compose.yml#L39 to mine blocks efficiently / in time
| "--confirm-external-bind", | ||
| "--add-exclusive-node=node_2:28080", | ||
| "--block-notify=/bin/sh ./scripts/notifier.sh -k -X GET https://host.docker.internal:14142/monerolikedaemoncallback/block?cryptoCode=xmr&hash=%s", | ||
| "--testnet", |
There was a problem hiding this comment.
| "--testnet", | |
| "--regtest", |
| "--add-exclusive-node=node_2:28080", | ||
| "--block-notify=/bin/sh ./scripts/notifier.sh -k -X GET https://host.docker.internal:14142/monerolikedaemoncallback/block?cryptoCode=xmr&hash=%s", | ||
| "--testnet", | ||
| "--no-igd", |
There was a problem hiding this comment.
| "--no-igd", |
removed upstream in v0.18.5.0
There was a problem hiding this comment.
| command: [ | ||
| "monero-wallet-rpc", | ||
| "--log-level=2", | ||
| "--allow-mismatched-daemon-version", |
they both use real pow, but testnet uses v1 transactions, and probably not even randomx? |
|
no need for randomx here.. i just need to pay for the invoice.. should be easy to switch/modify if we would need it tho |
|
Yea.. you said that regtest doesnt use pow.. it does.. and, it uses the same pow as mainnet. it also uses the same type of transactions. your old setup had a fixed difficulty of 1, the new setup has a fixed difficulty of 100. They both use pow |
|
but I also said it's 1:1 to prod.. testnet wont allow you to mine instantly for example.. (saves headache) |
yes it will.. you changed the difficulty from 1 on regtest to 150 on testnet |
|
yes.. to 150.. so it wont |
cab1802 to
51d0631
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe integration test environment now runs two Monero daemon nodes, routes the wallet through the second node, updates service dependencies and volumes, and replaces hardcoded wallet addresses and private view keys in RPC and UI test setup. ChangesMonero integration test environment
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
BTCPayServer.Plugins.IntegrationTests/docker-compose.yml (2)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
devservice pins a very old, EOLalpine:3.7(2017) base image.Not used by the test run itself, but anyone invoking
docker-compose up devgets a long-unsupported base. Consider a currentalpinetag instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BTCPayServer.Plugins.IntegrationTests/docker-compose.yml` around lines 26 - 32, Update the dev service image in the Docker Compose configuration from the EOL alpine:3.7 tag to a currently supported Alpine tag, leaving the service command and dependencies unchanged.
110-111: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider pinning to the latest
18.xpatch instead of18.1.PostgreSQL 18.4, 17.10, 16.14, 15.18, and 14.23 were released May 14, 2026, and each subsequent minor release included security fixes over 18.1 (e.g. "an update to all supported versions of PostgreSQL, including 18.1, 17.7, 16.11, 15.15, 14.20, and 13.23" that "fixes 2 security vulnerabilities and over 50 bugs"). Since this is a fresh pin (not an in-place upgrade), bumping straight to the latest
18.xpatch costs nothing extra.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BTCPayServer.Plugins.IntegrationTests/docker-compose.yml` around lines 110 - 111, Update the PostgreSQL image tag in the docker-compose service from 18.1 to the latest available 18.x patch release, keeping the existing postgres service configuration unchanged.BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.cs (1)
246-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared test Monero address/view-key into one constant. The same raw testnet address + view-key literal is duplicated across 5 locations in 2 files; past reviews already flagged this repeatedly ("Same"). Every key rotation (like this testnet swap) has to be edited in lockstep across all sites, which is exactly what happened here and is easy to get wrong.
BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.cs#L246-L247: defineinternal const string TestPrimaryAddressandTestPrivateViewKeyhere (or in a shared test fixtures class) and reference them from thegenerate_from_keysrequest params.BTCPayServer.Plugins.IntegrationTests/Monero/MoneroPluginIntegrationTest.cs#L43-L45: replace the literalFillAsyncarguments with the shared constants.BTCPayServer.Plugins.IntegrationTests/Monero/MoneroPluginIntegrationTest.cs#L124-L124: replace the literalFillAsyncargument with the shared view-key constant.BTCPayServer.Plugins.IntegrationTests/Monero/MoneroPluginIntegrationTest.cs#L146-L147: replace theGenerateFromKeysRequest.PrimaryAddress/PrivateViewKeyliterals with the shared constants.BTCPayServer.Plugins.IntegrationTests/Monero/MoneroPluginIntegrationTest.cs#L157-L159: replace the literalFillAsyncarguments with the shared constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.cs` around lines 246 - 247, Extract the duplicated Monero test address and view key into internal constants named TestPrimaryAddress and TestPrivateViewKey near the generate_from_keys request parameters in BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.cs (anchor lines 246-247). Replace the literals with these constants in MoneroPluginIntegrationTest.cs at lines 43-45, 124, 146-147, and 157-159, including FillAsync calls and GenerateFromKeysRequest fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BTCPayServer.Plugins.IntegrationTests/docker-compose.yml`:
- Around line 33-51: Update the --block-notify configuration in the node_1
monerod command to target node_2, matching the daemon used by xmr_wallet.
Preserve the existing callback URL and notification arguments while replacing
the notifier source so wallet confirmations remain synchronized.
---
Nitpick comments:
In `@BTCPayServer.Plugins.IntegrationTests/docker-compose.yml`:
- Around line 26-32: Update the dev service image in the Docker Compose
configuration from the EOL alpine:3.7 tag to a currently supported Alpine tag,
leaving the service command and dependencies unchanged.
- Around line 110-111: Update the PostgreSQL image tag in the docker-compose
service from 18.1 to the latest available 18.x patch release, keeping the
existing postgres service configuration unchanged.
In `@BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.cs`:
- Around line 246-247: Extract the duplicated Monero test address and view key
into internal constants named TestPrimaryAddress and TestPrivateViewKey near the
generate_from_keys request parameters in
BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.cs (anchor
lines 246-247). Replace the literals with these constants in
MoneroPluginIntegrationTest.cs at lines 43-45, 124, 146-147, and 157-159,
including FillAsync calls and GenerateFromKeysRequest fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bd817b14-3bc9-4c64-b1e9-984527d616e6
📒 Files selected for processing (3)
BTCPayServer.Plugins.IntegrationTests/Monero/IntegrationTestUtils.csBTCPayServer.Plugins.IntegrationTests/Monero/MoneroPluginIntegrationTest.csBTCPayServer.Plugins.IntegrationTests/docker-compose.yml
51d0631 to
fe248f1
Compare
fe248f1 to
b069001
Compare
|
One feature regtest offers is the ability to mine a number of blocks deterministically. I would expect this to be useful for tests. What feature of testnet is driving the switch here? It's not currently clear to me why we would move. https://docs.getmonero.org/rpc-library/monerod-rpc/#generateblocks |
Again, this is wrong. Unless you run modified daemon build with an updated hardfork table, you are NOT running a realistic copy of mainnet - you are running a copy of v1 monero. |
|
I switched to testnet so nobody after me would have a brilliant idea of instant mining and yet.. couple comments later we miss this feature.. have you folks seen such an ability on mainnet..? I did not.. to prevent anyone to go this direction I decided to switch.. (this have been working for haveno for years now) correct me if I'm wrong but if smthng works on v1 it should work on v16 (unless hard fork occurred) |
Instant mining works on a private test/stage/main/regtest. You have to send --fixed-difficulty=1 for that. Regtest difficulty raises just like testnet if you dont set a static difficulty. You, yourself, set a difficulty of 150 on your private testnet for this very reason.
Basicswap has been using regtest for years as well. (Haveno also has a modified hardfork table.)
each of those versions is a hard fork, and no, not everything from v1 works on v16 - thats the point of the hard forks. v14 transactions (11 ring members, no view tag) dont even work on v16. v1 doesnt have ringct (it has transparent amounts and bucketing), bulletproofs (or bp+), randomx, the list goes on. Again, haveno uses a modified HF table |
|
We can modify the table as well if needed. |
So you'd rather needlessly build a custom monerod daemon, creates coinbases from each hard fork, etc, just so that you can use testnet addresses? Because there is, afaict, zero reason to prefer a private-testnet over regtest |
just wrote you reason..
No.. not really.. I just want to fix production bug - additional payments.. nothing more.. once that is done we should focus on fees for example.. and I don't know yet but there is a change that the fees would be much better suited for regtest as one can easily mock/stub there.. but for now this feels like the best option to me.. and it's a minor thingy |
|
This should be regtest Napoly. Comments clearly state this. You have now ignored and merged. |
3 participants
https://moneroexamples.github.io/private-testnet/
related to: btcpay-monero/monero-csharp#80
Summary by CodeRabbit