Possible crash when (not) writing the compact size of the script/prevector

[Sjors]

Discovered while testing the Template Provider.

Sjors/bitcoin#71 (comment)

There's a chance this bug isn't in multiprocess but in a rebasing or other mistake on my end, but it's probably worth looking into.

[maflcko]

You can reproduce with bitcoin/bitcoin#30437 and

diff --git a/src/bitcoin-mine.cpp b/src/bitcoin-mine.cpp
index 4d5faea23e..08281b57d4 100644
--- a/src/bitcoin-mine.cpp
+++ b/src/bitcoin-mine.cpp
@@ -120,5 +120,7 @@ MAIN_FUNCTION
         tfm::format(std::cout, "Tip hash is null.\n");
     }
 
+    (void)mining->createNewBlock({});
+
     return EXIT_SUCCESS;
 }

[Sjors]

@maflcko does it crash with {CScript() << OP_TRUE} as well?

[maflcko]

Yes, with:

==48391==    by 0x11CCFD7: SpanReader::read(Span<std::byte>) 
==48391==    by 0x9466E5: ParamsStream<SpanReader&, TransactionSerParams>::read(Span<std::byte>) (serialize.h:1134)
==48391==    by 0x991269: void Unserialize<ParamsStream<SpanReader&, TransactionSerParams>, 28u, unsigned char>(ParamsStream<SpanReader&, TransactionSerParams>&, prevector<28u, unsigned char, unsigned int, int>&) (serialize.h:830)
==48391==    by 0x99117D: void UnserializeMany<ParamsStream<SpanReader&, TransactionSerParams>, prevector<28u, unsigned char, unsigned int, int>&>(ParamsStream<SpanReader&, TransactionSerParams>&, prevector<28u, unsigned char, unsigned int, int>&) (serialize.h:1000)
==48391==    by 0x99112D: void ActionUnserialize::SerReadWriteMany<ParamsStream<SpanReader&, TransactionSerParams>, prevector<28u, unsigned char, unsigned int, int>&>(ParamsStream<SpanReader&, TransactionSerParams>&, prevector<28u, unsigned char, unsigned int, int>&) (serialize.h:1032)
==48391==    by 0x9910DD: void CScript::SerializationOps<ParamsStream<SpanReader&, TransactionSerParams>, CScript, ActionUnserialize>(CScript&, ParamsStream<SpanReader&, TransactionSerParams>&, ActionUnserialize) (script.h:465)
==48391==    by 0x99107D: void CScript::Unser<ParamsStream<SpanReader&, TransactionSerParams> >(ParamsStream<SpanReader&, TransactionSerParams>&, CScript&) (script.h:465)
==48391==    by 0x99102D: void CScript::Unserialize<ParamsStream<SpanReader&, TransactionSerParams> >(ParamsStream<SpanReader&, TransactionSerParams>&) (script.h:465)
==48391==    by 0xA1049F: auto decltype(auto) mp::CustomReadField<CScript, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>, mp::ReadDestEmplace<CScript, mp::PassField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, CScript const&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&> >(mp::Priority<0>, mp::TypeList<CScript const&>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&>&&)::{lambda((auto:1&&)...)#1}> >(mp::TypeList<CScript>, mp::Priority<1>, mp::InvokeContext&, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>&&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&&) requires (Unserializable<CScript, DataStream>)&&(!(ipc::capnp::Deserializable<CScript>))::{lambda(auto:1&)#1}::operator()<CScript>(CScript&) const (common-types.h:78)
==48391==    by 0xA1037A: decltype(auto) mp::ReadDestEmplace<CScript, mp::PassField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, CScript const&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&> >(mp::Priority<0>, mp::TypeList<CScript const&>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&>&&)::{lambda((auto:1&&)...)#1}>::update<decltype(auto) mp::CustomReadField<CScript, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>, mp::ReadDestEmplace<CScript, mp::PassField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, CScript const&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&> >(mp::Priority<0>, mp::TypeList<CScript const&>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&>&&)::{lambda((auto:1&&)...)#1}> >(mp::TypeList<CScript>, mp::Priority<1>, mp::InvokeContext&, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>&&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&&) requires (Unserializable<CScript, DataStream>)&&(!(ipc::capnp::Deserializable<CScript>))::{lambda(auto:1&)#1}>(decltype(auto) mp::CustomReadField<CScript, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>, mp::ReadDestEmplace<CScript, mp::PassField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, CScript const&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&> >(mp::Priority<0>, mp::TypeList<CScript const&>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&>&&)::{lambda((auto:1&&)...)#1}> >(mp::TypeList<CScript>, mp::Priority<1>, mp::InvokeContext&, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>&&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&&) requires (Unserializable<CScript, DataStream>)&&(!(ipc::capnp::Deserializable<CScript>))::{lambda(auto:1&)#1}&&) (proxy-types.h:266)
==48391==    by 0xA10319: decltype(auto) mp::CustomReadField<CScript, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>, mp::ReadDestEmplace<CScript, mp::PassField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, CScript const&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&> >(mp::Priority<0>, mp::TypeList<CScript const&>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&, mp::ServerField<1, mp::Accessor<mp::mining_fields::Options, 17>, mp::ServerRet<mp::Accessor<mp::mining_fields::Result, 18>, mp::ServerCall> > const&, mp::TypeList<node::BlockCreateOptions const&>&&)::{lambda((auto:1&&)...)#1}> >(mp::TypeList<CScript>, mp::Priority<1>, mp::InvokeContext&, mp::StructField<mp::Accessor<mp::mining_fields::ScriptPubKey, 17>, ipc::capnp::messages::Mining::CreateNewBlockParams::Reader const>&&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Mining>, capnp::CallContext<ipc::capnp::messages::Mining::CreateNewBlockParams, ipc::capnp::messages::Mining::CreateNewBlockResults> >&&) requires (Unserializable<CScript, DataStream>)&&(!(ipc::capnp::Deserializable<CScript>)) (common-types.h:73)

[ryanofsky]

I'm guessing serialization code is confused by CScript being convertible to a span of bytes but also having serialize and deserialize methods. So it is being sent as raw bytes:

https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/src/ipc/capnp/common-types.h#L151-L157

which receiving code tries to pass to Unserialize

https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/src/ipc/capnp/common-types.h#L69-L80

Probably the easiest way to fix this would be to add a Priority<2> CustomReadField for CScript that calls the CScript constructor constructing it from raw bytes:

https://github.com/bitcoin/bitcoin/blob/ebe4cac38bf6c510b00b8e080acab079c54016d6/src/script/script.h#L463

Alternately, it might make sense for cscript to have a std::span<std::byte> constructor and for there to be a generic CustomReadField function which calls that constructor

[maflcko]

I am not sure about fixing this with additional bandages on top. If code can lead to runtime errors, it should ideally fail compilation in the first place. Otherwise, this seems like a cat-and-mouse game where a user runs into a random crash and then has to wait for developers to fix it in the next release, if it is reported at all. Or even worse, a roundrtip mismatch will silently corrupt the data.

My suggested fix would be:

• Use the same requires-constraints for serialization and deserialization, to ensure that serialize.h-serialization or span-serialization is used both ways, or not at all.
• If neither is available for a consistent round-trip serialization, fail compilation

[ryanofsky]

Those are good thoughts.

I do think this problem is mostly just caused by the unusual CustomBuildField span<byte> overload. Normally Build/Read overloads (or Build/Pass overloads) are paired together, but this Build overload is unpaired. I originally added it to support passing GCSFilter::ElementSet arguments, and it got used more places after that like BlockTemplate::getCoinbaseCommitment method currently, and I never got around to cleaning it up and implementing TODO there. I think if I added a corresponding Read overload to that Build overload, and restricted them both to only work on the same types, it would be a reasonable and safe fix for this problem.

Another possible improvement and fix could be to change capnp proto schema. Instead of allowing bitcoin serialization be applied to any capnp Data field, the schema could define a new type like struct SerializedData { data @0 :Data; } and only allow bitcoin serialization to and from SerializedData fields instead of Data fields. This would add a little bit extra overhead to the representation though, which is why I didn't implement it that way initially.

The fix you suggest also could make sense but I would have to think more about how to implement it.