💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471915202)
yes, fixed. Thanks. God knows why I did that.. it seems that I wasn't focused.
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471915202)
yes, fixed. Thanks. God knows why I did that.. it seems that I wasn't focused.
💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471915666)
yeah, fixed. I pasted your doc suggestion first, then made the classes consolidation..
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471915666)
yeah, fixed. I pasted your doc suggestion first, then made the classes consolidation..
📝 sr-gi opened a pull request: "test: fixes flaky wallet_import_rescan functional test"
(https://github.com/bitcoin/bitcoin/pull/29350)
wallet_import_rescan is, potentially, not rounding values used for `sendtoaddress` in one of the test cases. Under normal conditions, the value is randomly picked and rounded to 8 decimal positions, however, for this specific case the value is limited to a maximum value that is not properly rounded. If that value happens to be picked, the test will fail.
I noticed this in a completely unrelated PR where the test failed: https://cirrus-ci.com/task/5467779164274688?logs=ci#L2684
<!--
*** Pl
...
(https://github.com/bitcoin/bitcoin/pull/29350)
wallet_import_rescan is, potentially, not rounding values used for `sendtoaddress` in one of the test cases. Under normal conditions, the value is randomly picked and rounded to 8 decimal positions, however, for this specific case the value is limited to a maximum value that is not properly rounded. If that value happens to be picked, the test will fail.
I noticed this in a completely unrelated PR where the test failed: https://cirrus-ci.com/task/5467779164274688?logs=ci#L2684
<!--
*** Pl
...
💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471941289)
All good, applied your suggestion. Thanks!
> This is exactly what I think the comment should not be saying because the error cannot be transient. And it's confusing to suggest that it could be transient, because it either makes the rest of the code seem broken, or it makes this code seem unnecessary.
I was also thinking about an I/O hardware malfunctioning error, which could be transient; sqlite has return codes for it. But it's probably better to abort the program if something like this e
...
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1471941289)
All good, applied your suggestion. Thanks!
> This is exactly what I think the comment should not be saying because the error cannot be transient. And it's confusing to suggest that it could be transient, because it either makes the rest of the code seem broken, or it makes this code seem unnecessary.
I was also thinking about an I/O hardware malfunctioning error, which could be transient; sqlite has return codes for it. But it's probably better to abort the program if something like this e
...
💬 Wwwwwwuuuuu commented on pull request "test: fixes flaky wallet_import_rescan functional test":
(https://github.com/bitcoin/bitcoin/pull/29350#issuecomment-1917873911)
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125
(https://github.com/bitcoin/bitcoin/pull/29350#issuecomment-1917873911)
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125
🤔 furszy reviewed a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1852350177)
Updated per feedback, thanks ryanofsky!
Docstrings were improved, and a bug related to a null `SQLiteExecHandler` has been fixed.
[Small diff](https://github.com/bitcoin/bitcoin/compare/756bed0529b46944162c02c783ecc804f802b637..b298242c8d495c36072415e1b95eaa7bf485a38a).
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1852350177)
Updated per feedback, thanks ryanofsky!
Docstrings were improved, and a bug related to a null `SQLiteExecHandler` has been fixed.
[Small diff](https://github.com/bitcoin/bitcoin/compare/756bed0529b46944162c02c783ecc804f802b637..b298242c8d495c36072415e1b95eaa7bf485a38a).
⚠️ Wwwwwwuuuuu opened an issue: "Please help me figure out why this transaction is not confirmed?"
(https://github.com/bitcoin/bitcoin/issues/29351)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [X] I still think this issue should be opened here
### Report
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125
(https://github.com/bitcoin/bitcoin/issues/29351)
### Issues, reports or feature requests related to the GUI should be opened directly on the GUI repo
- [X] I still think this issue should be opened here
### Report
Please help me figure out why this transaction is not confirmed?8f62acf0f0ddc3e38db2e8ec2755f4e54aa5bf616d685e2f3361155fc9d44125
🤔 furszy reviewed a pull request: "test: fixes flaky wallet_import_rescan functional test"
(https://github.com/bitcoin/bitcoin/pull/29350#pullrequestreview-1852354792)
See #29343.
(https://github.com/bitcoin/bitcoin/pull/29350#pullrequestreview-1852354792)
See #29343.
💬 furszy commented on pull request "test: fixes flaky wallet_import_rescan functional test":
(https://github.com/bitcoin/bitcoin/pull/29350#discussion_r1471953382)
you are overwriting the random number.
(https://github.com/bitcoin/bitcoin/pull/29350#discussion_r1471953382)
you are overwriting the random number.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1917887536)
The `p2p_v2_earlykeyresponse.py` test is unrelated.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1917887536)
The `p2p_v2_earlykeyresponse.py` test is unrelated.
✅ willcl-ark closed an issue: "Please help me figure out why this transaction is not confirmed?"
(https://github.com/bitcoin/bitcoin/issues/29351)
(https://github.com/bitcoin/bitcoin/issues/29351)
💬 willcl-ark commented on issue "Please help me figure out why this transaction is not confirmed?":
(https://github.com/bitcoin/bitcoin/issues/29351#issuecomment-1917891085)
(It looks like the fee is a bit low for current mempool conditions.)
This issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com/) or the #bitcoin IRC channel on the [Libera Chat](https://libera.chat/) network.
For proposed protocol changes you can post to the bitcoin-dev [mailing list](https://lists.linuxfoundation.org/mail
...
(https://github.com/bitcoin/bitcoin/issues/29351#issuecomment-1917891085)
(It looks like the fee is a bit low for current mempool conditions.)
This issue tracker is used to track technical issues related to the Bitcoin Core code base.
General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com/) or the #bitcoin IRC channel on the [Libera Chat](https://libera.chat/) network.
For proposed protocol changes you can post to the bitcoin-dev [mailing list](https://lists.linuxfoundation.org/mail
...
💬 sr-gi commented on pull request "test: fixes flaky wallet_import_rescan functional test":
(https://github.com/bitcoin/bitcoin/pull/29350#discussion_r1471957427)
Oh wait, that was me debugging, my bad
(https://github.com/bitcoin/bitcoin/pull/29350#discussion_r1471957427)
Oh wait, that was me debugging, my bad
✅ sr-gi closed a pull request: "test: fixes flaky wallet_import_rescan functional test"
(https://github.com/bitcoin/bitcoin/pull/29350)
(https://github.com/bitcoin/bitcoin/pull/29350)
💬 sr-gi commented on pull request "test: fixes flaky wallet_import_rescan functional test":
(https://github.com/bitcoin/bitcoin/pull/29350#issuecomment-1917894121)
> See #29343.
Ups, I missed that. Closing
(https://github.com/bitcoin/bitcoin/pull/29350#issuecomment-1917894121)
> See #29343.
Ups, I missed that. Closing
🤔 sr-gi reviewed a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343#pullrequestreview-1852388419)
ACK [26ad2ae](https://github.com/bitcoin/bitcoin/pull/29343/commits/26ad2aeb29dd0875e8509917ddaa586997e977d2)
I found this and got to a similar patch independently
(https://github.com/bitcoin/bitcoin/pull/29343#pullrequestreview-1852388419)
ACK [26ad2ae](https://github.com/bitcoin/bitcoin/pull/29343/commits/26ad2aeb29dd0875e8509917ddaa586997e977d2)
I found this and got to a similar patch independently
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917933292)
> WIP branch
Is this consistently failing for you (or can you at least reproduce it)? I can see it also failing on some of the current open PRs but I can not get my head around why. This should only happen if `PeerEarlyKey` receives any data from the node before sending anything at all, or after sending the network magic but before sending the remaining initial bytes + garbage.
To my understanding, the node should not have been able to parse enough data to be replying (with either v1 or v2
...
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917933292)
> WIP branch
Is this consistently failing for you (or can you at least reproduce it)? I can see it also failing on some of the current open PRs but I can not get my head around why. This should only happen if `PeerEarlyKey` receives any data from the node before sending anything at all, or after sending the network magic but before sending the remaining initial bytes + garbage.
To my understanding, the node should not have been able to parse enough data to be replying (with either v1 or v2
...
🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1852266688)
Light Code Review ACK 27f260aa6e04f82dad78e9a06d58927546143a27
I don't have a strong opinion on the interface issue (I read the discussion and can see both points of view), but I do think that having a global variable `g_initial_block_download_completed` that lives in `protocol.h` and is updated dynamically from `net_processing` is an ugly layer violation too, so removing that is an important improvement from the architectural point of view.
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1852266688)
Light Code Review ACK 27f260aa6e04f82dad78e9a06d58927546143a27
I don't have a strong opinion on the interface issue (I read the discussion and can see both points of view), but I do think that having a global variable `g_initial_block_download_completed` that lives in `protocol.h` and is updated dynamically from `net_processing` is an ugly layer violation too, so removing that is an important improvement from the architectural point of view.
💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1471891912)
could also have a check for `HasAllDesirableServiceFlags` each time `GetDesirableServiceFlags` is checked.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1471891912)
could also have a check for `HasAllDesirableServiceFlags` each time `GetDesirableServiceFlags` is checked.
💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917980494)
> So it seems like there is a larger bug here
Yes, that could be the case. Since I don't see how this can correct without restarting after the blocks are connecte to the chain and no longer assumed valid, I wonder if that could make the existing check https://github.com/bitcoin/bitcoin/blob/cad2df24b396be403f13f372ec48ea14a90d7f06/src/validation.cpp#L4949 fail.
Since it is very slow to sync with `-checkblockindex=1` even on signet (though commit https://github.com/bitcoin/bitcoin/pull/28339/
...
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1917980494)
> So it seems like there is a larger bug here
Yes, that could be the case. Since I don't see how this can correct without restarting after the blocks are connecte to the chain and no longer assumed valid, I wonder if that could make the existing check https://github.com/bitcoin/bitcoin/blob/cad2df24b396be403f13f372ec48ea14a90d7f06/src/validation.cpp#L4949 fail.
Since it is very slow to sync with `-checkblockindex=1` even on signet (though commit https://github.com/bitcoin/bitcoin/pull/28339/
...