MetacoSA / NBitcoin

Comprehensive Bitcoin library for the .NET framework.
MIT License
1.86k stars 840 forks source link

[BUG] `InventoryVector` should not inherit from `Payload` #1133

Closed torx-cz closed 1 year ago

torx-cz commented 1 year ago

Hello everyone! I am working with NBitcoin, it is good library, but i found something, that i think is bug: public class InventoryVector : Payload, IBitcoinSerializable ref: https://github.com/MetacoSA/NBitcoin/blob/602dea5ed312e5a7e3bca639f39690cf185b3590/NBitcoin/Protocol/InventoryVector.cs#L25

Why class InventoryVector inherit from Payload? I think that InventoryVector is not stand alone Bitcoin message, it is only part of the message (e.g. InventoryPayload) - so I think that InventoryVector is not Payload - it is only IBitcoinSerializable - it doesn't have Command (which Payload expect).

The reason I found that is, it isn't json serializable: code:

var invVector = new InventoryVector(InventoryType.MSG_TX, uint256.One);
Console.WriteLine(JsonSerializer.Serialize(invVector));

Exception:

Unhandled exception. System.ArgumentException: NBitcoin.Protocol.InventoryVector is not a payload
   at NBitcoin.Protocol.PayloadAttribute.GetCommandName(Type type)
   at NBitcoin.Protocol.Payload.get_Command()
   at System.Text.Json.Serialization.Metadata.JsonPropertyInfo`1.GetMemberAndWriteJson(Object obj, WriteStack& state, Utf8JsonWriter writer)
   at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.TryWrite(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.Serialization.JsonConverter`1.WriteCore(Utf8JsonWriter writer, T& value, JsonSerializerOptions options, WriteStack& state)
   at System.Text.Json.JsonSerializer.WriteUsingSerializer[TValue](Utf8JsonWriter writer, TValue& value, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.WriteStringUsingSerializer[TValue](TValue& value, JsonTypeInfo jsonTypeInfo)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)

Thank you for helping me! :)

NicolasDorier commented 1 year ago

I think you are right, can you make a PR? tests will fail if we are wrong.