Bump NBitcoin to 10.0.9 and NBXplorer.Client to 5.0.8 - #572
Open
Jossec101 wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates NodeGuard’s Bitcoin/NBXplorer dependencies to NBitcoin 10 / NBXplorer.Client 5.0.8 (net10.0), and replaces removed NBitcoin OutputDescriptor APIs with a local, limited output-descriptor parser/renderer to preserve NodeGuard’s supported descriptor surface and checksum behavior.
Changes:
- Bump
NBitcointo10.0.9,NBXplorer.Clientto5.0.8, andNBitcoin.TestFrameworkto5.0.2. - Rework
WalletParserto parse/render the supported descriptor subset without relying on removedNBitcoin.Scripting.OutputDescriptor. - Update wallet lexicographical-order test to rebuild
sortedmulti(...)from extracted multisig script parameters.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/NodeGuard.Tests/NodeGuard.Tests.csproj | Bumps NBitcoin.TestFramework to align with NBitcoin 10 dependency chain. |
| test/NodeGuard.Tests/Data/Models/WalletTests.cs | Replaces removed OutputDescriptor.InferFromScript usage with multisig script parameter extraction. |
| src/NodeGuard.csproj | Updates NBitcoin and NBXplorer.Client package versions to required net10.0-compatible releases. |
| src/Helpers/WalletParser.cs | Implements a custom descriptor parser and manual descriptor rendering with checksum support. |
| src/Data/Models/Wallet.cs | Removes obsolete NBitcoin.Scripting import after NBitcoin 10 upgrade. |
Suppressed comments (1)
src/Helpers/WalletParser.cs:142
- ExtractFromKey returns
nullfor the returned key-origin tuple array when no origin is provided. Callers (and ExtractFromMulti via SelectMany) assume this array is non-null, which can lead to NullReferenceException or confusing import failures for descriptors without "[fingerprint/path]" origins. It’s safer to reject keys without origin info with a clear FormatException.
if (keyOriginInfo == null)
return (strategy, null);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+12
to
+14
| /// NodeGuard accepts a deliberately small descriptor grammar: wpkh/pkh single-sig and | ||
| /// (w)sh-less multi/sortedmulti or wsh(multi/sortedmulti), with external-chain-only keys | ||
| /// ("xpub" or "xpub/0/*"). NBitcoin 10 removed the NBitcoin.Scripting OutputDescriptor API and |
Comment on lines
+52
to
54
| case "sh": | ||
| throw new FormatException( | ||
| "Legacy multisig is not supported, please use segwit multisig instead."); |
Comment on lines
+206
to
+208
| WalletAddressType.NativeSegwit => $"wsh({multi})", | ||
| WalletAddressType.NestedSegwit => $"sh({multi})", | ||
| WalletAddressType.Legacy => multi, |
RodriFS
previously approved these changes
Aug 19, 2026
Jossec101
force-pushed
the
deps/nbitcoin-10
branch
from
August 24, 2026 14:39
9554244 to
1cfe32f
Compare
Jossec101
force-pushed
the
deps/nbitcoin-10
branch
from
August 26, 2026 16:12
1cfe32f to
9c48715
Compare
Paired bump: NBXplorer.Client 5.0.8 targets net10.0 and requires NBitcoin >= 10.0.8. Also NBitcoin.TestFramework 4.0.2 -> 5.0.2. NBitcoin 10 removed the NBitcoin.Scripting OutputDescriptor API; its Miniscript/WalletPolicies replacement only parses BIP388 multipath descriptors (/**), which NodeGuard's watch-only import explicitly rejects (only external-chain /0/* keys are accepted). To preserve the exact accepted grammar and error behavior, WalletParser now parses the small supported descriptor surface directly (wpkh/pkh/multi/sortedmulti/ wsh(multi), origins, checksum validation) and renders export descriptors manually with Miniscript.AddChecksum for the BIP380 checksum. The lexicographical-order test rebuilds the descriptor from the multisig script parameters since OutputDescriptor.InferFromScript is gone. All 389 tests pass, including the pinned descriptor parse/export vectors (checksums, key ordering, error types).
Jossec101
force-pushed
the
deps/nbitcoin-10
branch
from
August 28, 2026 09:22
9c48715 to
915f1ad
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Paired bump: NBXplorer.Client 5.0.8 targets net10.0 and requires
NBitcoin >= 10.0.8. Also NBitcoin.TestFramework 4.0.2 -> 5.0.2.
NBitcoin 10 removed the NBitcoin.Scripting OutputDescriptor API; its
Miniscript/WalletPolicies replacement only parses BIP388 multipath
descriptors (/**), which NodeGuard's watch-only import explicitly rejects
(only external-chain /0/* keys are accepted). To preserve the exact
accepted grammar and error behavior, WalletParser now parses the small
supported descriptor surface directly (wpkh/pkh/multi/sortedmulti/
wsh(multi), origins, checksum validation) and renders export descriptors
manually with Miniscript.AddChecksum for the BIP380 checksum. The
lexicographical-order test rebuilds the descriptor from the multisig
script parameters since OutputDescriptor.InferFromScript is gone.
All 389 tests pass, including the pinned descriptor parse/export vectors
(checksums, key ordering, error types).
Stack created with GitHub Stacks CLI • Give Feedback 💬