💬 ChrisCho-H commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1645528886)
Thx! I updated
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1645528886)
Thx! I updated
💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270656464)
> If ProcessMessagesOnce() is called directly, then it will not be an end-to-end test. It will defeat the purpose of this PR.
Makes sense. I guess I misunderstood the goal of this pull. So the goal is to mimic the functional tests that already do the same checks, but do it without spinning up a real socket, but use a mocked socket?
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1270656464)
> If ProcessMessagesOnce() is called directly, then it will not be an end-to-end test. It will defeat the purpose of this PR.
Makes sense. I guess I misunderstood the goal of this pull. So the goal is to mimic the functional tests that already do the same checks, but do it without spinning up a real socket, but use a mocked socket?
📝 MarcoFalke opened a pull request: "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC"
(https://github.com/bitcoin/bitcoin/pull/28118)
There should be no risk in adding a call to `SyncWithValidationInterfaceQueue` here. In fact, it will make tests less brittle. For example,
* If one sets the timeouts in `test/functional/feature_fee_estimation.py` to `0`, on `master` the test will fail and here it will pass.
* It may avoid a rare (theoretic) intermittent issue in https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663
(https://github.com/bitcoin/bitcoin/pull/28118)
There should be no risk in adding a call to `SyncWithValidationInterfaceQueue` here. In fact, it will make tests less brittle. For example,
* If one sets the timeouts in `test/functional/feature_fee_estimation.py` to `0`, on `master` the test will fail and here it will pass.
* It may avoid a rare (theoretic) intermittent issue in https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663
💬 MarcoFalke commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1270660411)
Attempt in https://github.com/bitcoin/bitcoin/pull/28118
(https://github.com/bitcoin/bitcoin/pull/28108#discussion_r1270660411)
Attempt in https://github.com/bitcoin/bitcoin/pull/28118
💬 jonatack commented on pull request "ci: Start with clean env":
(https://github.com/bitcoin/bitcoin/pull/27976#issuecomment-1645571335)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/27976#issuecomment-1645571335)
Approach ACK
💬 instagibbs commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1270671380)
This logging should be handled by the caller, since the caller sometimes isn't actually requesting the items!
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1270671380)
This logging should be handled by the caller, since the caller sometimes isn't actually requesting the items!
💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1268163179)
26a7c254dcb49a92070c88cc7963e4996cd9e7c7
(Non-blocking note:) a little unexpected that this is written out of sequence with how it's described in [RFC 8439](https://datatracker.ietf.org/doc/html/rfc8439#section-2.8);
> - First, a Poly1305 one-time key is generated from the 256-bit key and nonce using the procedure described in [Section 2.6](https://datatracker.ietf.org/doc/html/rfc8439#section-2.6).
> - Next, the ChaCha20 encryption function is called to encrypt the plaintext, using the
...
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1268163179)
26a7c254dcb49a92070c88cc7963e4996cd9e7c7
(Non-blocking note:) a little unexpected that this is written out of sequence with how it's described in [RFC 8439](https://datatracker.ietf.org/doc/html/rfc8439#section-2.8);
> - First, a Poly1305 one-time key is generated from the 256-bit key and nonce using the procedure described in [Section 2.6](https://datatracker.ietf.org/doc/html/rfc8439#section-2.6).
> - Next, the ChaCha20 encryption function is called to encrypt the plaintext, using the
...
🤔 jamesob reviewed a pull request: "BIP324 ciphersuite"
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1537240624)
Submitting partial review (now to some degree obviated) up to the `FSChaCha20` implementation.
Looking good so far. I also did cross-validation of the test vectors with [a Python library](https://github.com/ph4r05/py-chacha20poly1305) (happily a different implementation than The Stack Man) - the script for that can be found [here](https://github.com/jamesob/bitcoin-review-data/blob/master/28008.sipa.bip324_ciphersuite/verifycrypto.py), where I've manually copied test data from @sipa's cpp and
...
(https://github.com/bitcoin/bitcoin/pull/28008#pullrequestreview-1537240624)
Submitting partial review (now to some degree obviated) up to the `FSChaCha20` implementation.
Looking good so far. I also did cross-validation of the test vectors with [a Python library](https://github.com/ph4r05/py-chacha20poly1305) (happily a different implementation than The Stack Man) - the script for that can be found [here](https://github.com/jamesob/bitcoin-review-data/blob/master/28008.sipa.bip324_ciphersuite/verifycrypto.py), where I've manually copied test data from @sipa's cpp and
...
💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1268291488)
Note that this ciphertext includes the Poly1305 tag; in the RFC, ciphertext vector omits the tag.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1268291488)
Note that this ciphertext includes the Poly1305 tag; in the RFC, ciphertext vector omits the tag.
💬 jamesob commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1268282951)
Note for reviewers that RFC test vectors use network byte order (big-endian); the `Nonce96` constructor here expects little-endian representation. Hence the reversal relative to RFC vectors.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1268282951)
Note for reviewers that RFC test vectors use network byte order (big-endian); the `Nonce96` constructor here expects little-endian representation. Hence the reversal relative to RFC vectors.
💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1645619519)
Ok, going to close this for now then. Looks like next steps are to open an issue upstream with binutils, and report the (potential) false positive.
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1645619519)
Ok, going to close this for now then. Looks like next steps are to open an issue upstream with binutils, and report the (potential) false positive.
✅ fanquake closed an issue: ""missing operand" assembler warnings on Windows"
(https://github.com/bitcoin/bitcoin/issues/28111)
(https://github.com/bitcoin/bitcoin/issues/28111)
⚠️ manfreddd opened an issue: "Bitcoin Core v25.0 Crashes"
(https://github.com/bitcoin/bitcoin/issues/28119)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Bitcoin Core keeps crashing and won't stay up. I have been running a Bitcoin node on a Windows 10 computer for several years and had never experienced a Bitcoin Core crash before.
I had been running Bitcoin Core v23.0 since if first came out but left the node unattended for a few weeks this summer. I restarted it earlier this week and the Bitcoin Core log window showed its data base wa
...
(https://github.com/bitcoin/bitcoin/issues/28119)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Bitcoin Core keeps crashing and won't stay up. I have been running a Bitcoin node on a Windows 10 computer for several years and had never experienced a Bitcoin Core crash before.
I had been running Bitcoin Core v23.0 since if first came out but left the node unattended for a few weeks this summer. I restarted it earlier this week and the Bitcoin Core log window showed its data base wa
...
📝 furszy opened a pull request: "p2p: make block download logic aware of limited peers threshold"
(https://github.com/bitcoin/bitcoin/pull/28120)
Even when the node believes it has IBD completed, need to avoid
requesting historical blocks from network-limited peers.
Otherwise, the limited peer will disconnect right away.
The simplest scenario could be a node that gets synced, drop
connections and stay inactive for a while. Then, once it re-connects,
as IBD is completed, the node tries to fetch all the missing
blocks from any peer. Getting disconnected by the limited ones.
(https://github.com/bitcoin/bitcoin/pull/28120)
Even when the node believes it has IBD completed, need to avoid
requesting historical blocks from network-limited peers.
Otherwise, the limited peer will disconnect right away.
The simplest scenario could be a node that gets synced, drop
connections and stay inactive for a while. Then, once it re-connects,
as IBD is completed, the node tries to fetch all the missing
blocks from any peer. Getting disconnected by the limited ones.
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270710501)
Close; the RFC (and its test vectors) treat the nonce as a 12-byte array, not a number. It's just on our side that we treat the nonce as a 32-bit and a 64-bit number (whose little endian encoding becomes the nonce in the ChaCha20 sense). So there is no big endian encoding anywhere (except in how integer constants are written in C++).
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270710501)
Close; the RFC (and its test vectors) treat the nonce as a 12-byte array, not a number. It's just on our side that we treat the nonce as a 32-bit and a 64-bit number (whose little endian encoding becomes the nonce in the ChaCha20 sense). So there is no big endian encoding anywhere (except in how integer constants are written in C++).
💬 sipa commented on pull request "BIP324 ciphersuite":
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270711568)
I believe this may be clearer now that it's been rewritten.
(https://github.com/bitcoin/bitcoin/pull/28008#discussion_r1270711568)
I believe this may be clearer now that it's been rewritten.
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1270716655)
I agree that this seems redundant; the original call to `UnloadBlockIndex()` was added in #25740, and if I'm reading correctly I don't think it should have been necessary then either. @jamesob Wondering if you agree, or if we're overlooking something?
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1270716655)
I agree that this seems redundant; the original call to `UnloadBlockIndex()` was added in #25740, and if I'm reading correctly I don't think it should have been necessary then either. @jamesob Wondering if you agree, or if we're overlooking something?
💬 sdaftuar commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1270720379)
I agree that it would be good to add an assert to catch this earlier, but we discussed before that due to the different ways that chainstates can be constructed and the order in which snapshot block hashes can be passed in (relative to when the block index is populated on startup), that there wasn't a clear way to do this at an earlier point in the code.
I'll update the code comment though to make it more clear what will break if an invalid hash is passed in.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1270720379)
I agree that it would be good to add an assert to catch this earlier, but we discussed before that due to the different ways that chainstates can be constructed and the order in which snapshot block hashes can be passed in (relative to when the block index is populated on startup), that there wasn't a clear way to do this at an earlier point in the code.
I'll update the code comment though to make it more clear what will break if an invalid hash is passed in.
💬 MarcoFalke commented on issue "Bitcoin Core v25.0 Crashes":
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1645658066)
> 2023-07-21T03:41:15Z Setting file arg: dbcache = "4096"
Is there a reason why this is larger? IIRC, there is no data available that increasing it will improve performance on all available hardware. If other programs use 4 GB of memory, Bitcoin Core may run out of memory and crash.
Can you try again with less and then also check if block connection is faster? Currently it takes almost 2 minutes, which seems slow:
```
2023-07-21T05:59:12Z [bench] - Connect block: 45491.16ms [7951.41s (
...
(https://github.com/bitcoin/bitcoin/issues/28119#issuecomment-1645658066)
> 2023-07-21T03:41:15Z Setting file arg: dbcache = "4096"
Is there a reason why this is larger? IIRC, there is no data available that increasing it will improve performance on all available hardware. If other programs use 4 GB of memory, Bitcoin Core may run out of memory and crash.
Can you try again with less and then also check if block connection is faster? Currently it takes almost 2 minutes, which seems slow:
```
2023-07-21T05:59:12Z [bench] - Connect block: 45491.16ms [7951.41s (
...
💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1645661021)
It is possible for a connection to have more than one request (see: https://github.com/libevent/libevent/issues/1383#issuecomment-1383249281) or `evhttp_associate_new_request_with_connection` in the libevent code. It appears though that the requests are sequential so when one completes the next one can run. I modified the python script to test this, but it doesn't lead to a deadlock or crash because when the next request comes in, it uses the same `evhttp_connection*` and so the insert into `g_h
...
(https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1645661021)
It is possible for a connection to have more than one request (see: https://github.com/libevent/libevent/issues/1383#issuecomment-1383249281) or `evhttp_associate_new_request_with_connection` in the libevent code. It appears though that the requests are sequential so when one completes the next one can run. I modified the python script to test this, but it doesn't lead to a deadlock or crash because when the next request comes in, it uses the same `evhttp_connection*` and so the insert into `g_h
...