Don't try to read SLE with key 0 from the ledger:

[ximinez]

High Level Overview of Change

Debug builds are occasionally hitting an assert at Ledger.cpp line 436 (see #4341). The assert is at least 8 years old and appears to still be relevant. Ledger::read should not be called with an ID of 0.

NFTokenAcceptOffer transactions can be written (presumably incorrectly) in such a way that the NFTokenSellOffer and NFTokenBuyOffer fields can contain a 0 value. The preclaim function for this transaction did not validate the value before calling Ledger::read.

Note that this issue is not harmful in release builds. If an assert fails in a debug build, as it did in this case, it will stop the program. However, asserts are not included in release builds, so they are skipped. In this case, Ledger::read properly handles that case, and returns a nullptr if the ID is 0. NFTokenSellOffer::preclaim checks that nullptr and correctly returns tecOBJECT_NOT_FOUND, as it would if the transaction provided a valid ID which was not in the ledger.

This fix preserves that functionality, but checks for 0 in NFTokenSellOffer::preclaim before calling Ledger::read, and returns the same tecOBJECT_NOT_FOUND result. Thus, an amendment is not required.

Type of Change

• [X ] Bug fix (non-breaking change which fixes an issue)
• [X ] Refactor (non-breaking change that only restructures code)

[scottschurr]

Change looks good, and I think this is a good approach to avoiding an amendment. Thanks for finding and fixing the problem.

But I do think it would be good to add unit tests. I don't think the tests need to be extensive. The topmost commit here I think would be sufficient: https://github.com/scottschurr/rippled/commits/ed-readidzero

[ximinez]

Thanks for the tests! Super helpful. I also verified that the assert fires without the fix in place.

[scottschurr]

👍 Looks great! Thanks.