chore: Creating a unified mechanism to generate blocklists for ckBTC and ckETH #3401

THLO · 2025-01-10T14:22:35Z

This PR adds a Python script to generate blocklists for ckBTC and ckETH based on the OFAC SDN list.

There are some differences in the produced blocklist files compared to the current files:

Differences to the current ckBTC blocklist file:

The new blocklist file has a comment at the beginning about the script that was used to generate it.
The new blocklist file has a more concise error message for the invalid Bitcoin address in the OFAC SDN list.

Differences to the current ckETH blocklist file:

The new blocklist file has a comment at the beginning about the script that was used to generate it, replacing the old comment.
Since Ethereum addresses are case-insensitive, all addresses are now consistently in lower case.
The parameter from_address in the function is_blocked has been changed to address to make it consistent with the corresponding function for ckBTC.

Corresponding ticket: https://dfinity.atlassian.net/browse/XC-250

gregorydemay

Thanks a lot @THLO for making this script! Some minor comments and python newbie questions.

rs/cross-chain/scripts/generate_blocklist.py

gregorydemay · 2025-01-10T16:25:46Z

rs/cross-chain/scripts/generate_blocklist.py

+#
+# 2) Run this script as follows:
+#
+# python generate_blocklist.py [currency (BTC or ETH)] [path to the SDN.XML file]


could we specify the output file as a 3rd argument instead of using the constant BLOCKLIST_FILENAME?

Or alternatively, just write output to stdout?

Sure, I can introduce a third argument.
Note that the script writes stuff to stdout: The written file is supposed to be a valid Rust source file whereas the extracted addresses are written to stdout, which could be used for other purposes including sanity checks.

I added the argument.

gregorydemay · 2025-01-10T16:29:48Z

rs/cross-chain/scripts/generate_blocklist.py

+    blocklist_file.write('#[cfg(test)]\nmod tests;\n\nuse bitcoin::Address;\n\n')
+    blocklist_file.write('/// The script to generate this file, including information about the source data, can be found here:\n')
+    blocklist_file.write('/// /rs/cross-chain/scripts/generate_blocklist.py\n\n')
+    blocklist_file.write('/// BTC is not accepted from nor sent to addresses on this list.\n')
+    blocklist_file.write('/// NOTE: Keep it sorted!\n')
+    blocklist_file.write('pub const BTC_ADDRESS_BLOCKLIST: &[&str] = &[\n')


nit: is there a multiline string concept in Python, similar to what Rust has? This could simplify readability. Similar comment for the other preamble/postamble.

I think Python uses """ for multi-line strings

Fair point. I'll try to improve readability.

gregorydemay · 2025-01-10T16:33:30Z

rs/cross-chain/scripts/generate_blocklist.py

+def store_blocklist(currency, addresses):
+    blocklist_file = open(BLOCKLIST_FILENAME, 'w')
+
+    if currency == 'BTC':


nit: maybe that's overkill for now but it might make sense to have a writer per currency, so that we don't have to intertwine if/else statements for preamble/body/postamble

I thought that it would probably be overkill. I'll see what I can do (without spending too much effort).

It didn't take all that much effort. Please check out the new version!

gregorydemay · 2025-01-10T16:41:27Z

rs/cross-chain/scripts/generate_blocklist.py

+        if currency == 'ETH':
+            address = address.lower()
+        if address in INVALID_ADDRESSES:
+            blocklist_file.write('    // ' + address_prefix + '"' + address[offset:] + '"' + address_suffix + ' (Invalid address prefix)\n')


I'm not sure I would handle invalid addresses in that script:

Separation of concerns: the purpose of that script is to parse the SDN into a list of addresses that can be understood by our Rust code. Our Rust unit tests should catch any invalid address.

The stated reason there (Invalid address prefix) cannot work for all invalid addresses the script tries cannot really determine the reason why an address is invalid (it could try to parse the addresses but I'm not sure it's worth it, in particular because the parsing will be done by another library that we use in our canisters.)

WDYT?

I just added it for the sake of convenience. Otherwise, we always run into the issue that the Rust unit tests first need to catch the invalid address(es), which requires an additional update step.
I expect/hope that the guys compiling the list won't introduce many more invalid addresses.

But your comment regarding separation of concerns is valid, of course. If you feel that this concern trumps convenience, then I can remove the check.

I decided to remove the check for the sake of simplicity.

gregorydemay · 2025-01-10T16:50:52Z

rs/cross-chain/scripts/generate_blocklist.py

+    for address in addresses:
+        # Ethereum addresses are case-insensitive. By contrast, only Bech32 Bitcoin addresses are case-insensitive.
+        if currency == 'ETH':
+            address = address.lower()


I would leave the address as is: while the address is case-insensitive, the case sensitivity is used as an optional checksum (see EIP-55). Also I have the impression that the generated list may no longer be sorted and this also produces a large diff with the existing blocklist.

Agree with @gregorydemay that we shouldn't do the case conversion here.

Okay, I'll remove the check.

gregorydemay · 2025-01-10T16:54:09Z

rs/cross-chain/scripts/generate_blocklist.py

+    addresses = []
+
+    # Generate the currency-specific suffix.
+    currency_suffix = 'XBT' if currency == 'BTC' else 'ETH'


dumb question: why is XBT used for BTC?

"XBT" is the Bitcoin ticker that complies with the ISO 4217 standard. According to this standard, currencies not tied to a specific nation begin with the letter “X”, and "BT" stands for "Bitcoin". 🙂

Co-authored-by: gregorydemay <[email protected]>

lpahlavi · 2025-01-14T14:50:33Z

rs/cross-chain/scripts/generate_blocklist.py

+    blocklist_file.close()
+
+if __name__ == '__main__':  
+    if len(sys.argv) < 3:


Sorry, I'm a bit late to the party. Just a suggestion: the Python argparse library is pretty powerful and e.g. generates useful help/error message for the script CLI. Could be useful here to e.g. restrict currency in {BTC, ETH} and provide a default value for filename.

lpahlavi · 2025-01-14T14:53:08Z

rs/cross-chain/scripts/generate_blocklist.py

+}'''
+
+    def format_address(self, address):
+        return 'ethereum_address!("' + address[2:] + '")'


Also a small suggestion: I find f-strings generally easier to read, it is for sure also a matter of taste :)

THLO added 4 commits December 3, 2024 10:51

Updated the blocklist.

d4a909c

Added the initial version of the unified blocklist for ckBTC/ckETH.

bf724b8

Updated the unified ckBTC/ckETH blocklist generation script.

c6e4e12

Merge branch 'master' into tl/unify_blocklist_process

adb1729

THLO requested a review from a team as a code owner January 10, 2025 14:22

github-actions bot added the chore label Jan 10, 2025

gregorydemay reviewed Jan 10, 2025

View reviewed changes

THLO and others added 6 commits January 14, 2025 13:32

Update rs/cross-chain/scripts/generate_blocklist.py

f73e4f0

Co-authored-by: gregorydemay <[email protected]>

Merge branch 'master' into tl/unify_blocklist_process

27146ea

Added the output filename as a parameter.

8494933

Improved readability of written blocks of text.

f99099a

Removed the conversion to lower-case Ethereum addresses.

ba87833

Introduced handlers for the different networks.

1153461

lpahlavi reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Creating a unified mechanism to generate blocklists for ckBTC and ckETH #3401

chore: Creating a unified mechanism to generate blocklists for ckBTC and ckETH #3401

THLO commented Jan 10, 2025

gregorydemay left a comment

gregorydemay Jan 10, 2025

ninegua Jan 13, 2025

THLO Jan 14, 2025

THLO Jan 14, 2025

gregorydemay Jan 10, 2025

ninegua Jan 13, 2025

THLO Jan 14, 2025

THLO Jan 14, 2025

gregorydemay Jan 10, 2025

THLO Jan 14, 2025

THLO Jan 14, 2025

gregorydemay Jan 10, 2025

THLO Jan 14, 2025

THLO Jan 14, 2025

gregorydemay Jan 10, 2025

ninegua Jan 13, 2025

THLO Jan 14, 2025

THLO Jan 14, 2025

gregorydemay Jan 10, 2025

THLO Jan 14, 2025

lpahlavi Jan 14, 2025

lpahlavi Jan 14, 2025

chore: Creating a unified mechanism to generate blocklists for ckBTC and ckETH #3401

Are you sure you want to change the base?

chore: Creating a unified mechanism to generate blocklists for ckBTC and ckETH #3401

Conversation

THLO commented Jan 10, 2025

gregorydemay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment