chore: use bitcoin::Network instead of wrapper enum
#473
+220
−246
Conversation
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
JoseSK999
suggested changes
May 6, 2025
Davidson-Souza
requested changes
May 6, 2025
101b351 to
314834a
Compare
|
Could you please squash it? The original commit structure was nice to review. |
314834a to
f77c083
Compare
|
@luisschwab this branch has a conflict with |
c9c4704 to
7f68d27
Compare
|
Weird, it's failing the SSL test due to a timeout. @Davidson-Souza do you think this is related? main()
File "/home/runner/work/Floresta/Floresta/./tests/run_tests.py", line 168, in main
raise RuntimeError(f"Tests failed: {log_file.read()}")
RuntimeError: Tests failed: [TestSslInitialization 2025-05-07 18:03:48.389285] Created PKCS#8 key at /tmp/floresta-func-tests.fae515b/data/ssl/key.pem
[TestSslInitialization 2025-05-07 18:03:48.390980] Created self-signed certificate at /tmp/floresta-func-tests.fae515b/data/ssl/cert.pem
[FLORESTAD 2025-05-07 18:03:48.391434+00:00] Starting node 'florestad': /tmp/floresta-func-tests.fae515b/binaries/florestad --daemon --network=regtest --data-dir /tmp/floresta-func-tests.fae515b/data/TestSslInitialization --ssl-key-path /tmp/floresta-func-tests.fae515b/data/ssl/key.pem --ssl-cert-path /tmp/floresta-func-tests.fae515b/data/ssl/cert.pem
Traceback (most recent call last):
File "/home/runner/work/Floresta/Floresta/tests/florestad/ssl-test.py", line 53, in <module>
TestSslInitialization().main()
File "/home/runner/work/Floresta/Floresta/tests/test_framework/__init__.py", line 192, in main
self.run_test()
File "/home/runner/work/Floresta/Floresta/tests/florestad/ssl-test.py", line 34, in run_test
self.run_node(TestSslInitialization.nodes[0])
File "/home/runner/work/Floresta/Floresta/tests/test_framework/__init__.py", line 350, in run_node
node.rpc.wait_for_connections(opened=True)
File "/home/runner/work/Floresta/Floresta/tests/test_framework/rpc/base.py", line 280, in wait_for_connections
self.wait_for_connection(host, port, opened)
File "/home/runner/work/Floresta/Floresta/tests/test_framework/rpc/base.py", line 274, in wait_for_connection
raise TimeoutError(f"{host}:{port} not {state} after {timeout} seconds")
TimeoutError: 127.0.0.1:18442 not open after 10 seconds |
7f68d27 to
b056516
Compare
|
Seems like it was a transient CI failure, it's passing now ✅. |
Davidson-Souza
requested changes
May 7, 2025
|
Oh, these got lost in rebase hell. Fixing it now. |
b056516 to
647b1ff
Compare
JoseSK999
suggested changes
May 8, 2025
|
@JoseSK999 the commit history will get messy, I'll just make the changes locally and rebase. |
|
@JoseSK999 I left some comments on your reviews. Can you address them? |
|
I think I did all |
Previously we were using `unwrap` for things like chainstore operations and `Option` values unwrapping. I have replaced all unwraps in the file with proper error handling. Added the new `IncompleteTip` and `Database` error variants. Polished a bit the comments. Also instead of implementing `From` ChainStateBuilder for Chainstate I implement `TryFrom` (it's fallible, failing if the chainstore and chain params are not set).
If a peer disconnects, we re-do all inflight requests that we had with that peer. However, the code that does so may fail early if `redo_inflight_request` return an error. It is the case if we request for utreexo data or compact block filters without having a peer that supports those requests. This commit adds an early check that makes these functions act as a no-op in case we can't fulfill the request. It is not a problem to not re-request stuff here, since we already have some fallback logic that will try it again.
A couple of replacements of `unwrap` for `expect` with proper messages. In partial_chain.rs `get_validation_flags` now takes the hash directly from the caller, avoiding the `unwrap` (exactly as we do for `get_validation_flags` in chain_state.rs). In `get_leaf_hashes` (udata.rs) we instead take the TxOut directly. We also avoid computing the txid twice by passing it.
Add: A name for the CI functional workflow new check_installed function build flag to force florestad building fix: the florestad symlink now its directly from the actual dir, as a absolute path. cargo build will update the linked binary too. echoing the directory at the end so one just see the dir if the script ran fine
a58b918 to
8ede946
Compare
|
Nice! @Davidson-Souza you mentioned |
|
Doesn't CI cover this? |
|
Hm I don't think so |
We can do an enum for this in |
JoseSK999
approved these changes
May 9, 2025
luisschwab
added a commit
to luisschwab/bdk-floresta
that referenced
this pull request
May 10, 2025
15 tasks
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.
What is the purpose of this pull request?
Which crates are being modified?
Description
This PR swaps the crate's
Networkwrapper overbitcoin::Networkin favor of usingbitcoin::Networkdirectly. Sincebitcoin::Networkdoes not implementDefault, it was necessary to implement one forConfig.Handling errors caused by unsupported networks is outside of the scope of this PR. Cases where these need to be handled can be found via:
rg --type rust '// TODO: handle possible Err'.I also set the
AssumeValidforTestnet4as the hash of block80_000:0000000006af13c1117f3e2eb14f10eb9736e255713118cf7eb6659b1448efc1. Let me know if that's too recent so I can replace it with an older block.Notes to the reviewers
I wasn't able to run integration tests locally, not even on master, so let's see what happens on CI.
Author Checklist
just pcc(recommended but slower)just lint-features '-- -D warnings' && cargo test --release