✅ willcl-ark closed an issue: "berkeley database failed to open database environment"
(https://github.com/bitcoin/bitcoin/issues/29286)
(https://github.com/bitcoin/bitcoin/issues/29286)
💬 willcl-ark commented on issue "berkeley database failed to open database environment":
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1904555895)
OK great. I'll close this for now then.
Do open a new issue if it happens again and you confirm that the wallet database (directory) is present, but failing to load.
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1904555895)
OK great. I'll close this for now then.
Do open a new issue if it happens again and you confirm that the wallet database (directory) is present, but failing to load.
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1462248344)
I actually had this as `params=[]` a few updates ago but that fails lint (see below) but I'll clean it up with a different work-around:
```
A mutable list or dict seems to be used as default parameter value:
test/functional/interface_rpc.py:41:def expect_http_rpc_status(expected_http_status, expected_rpc_error_code, node, method, params={}):
This is how mutable list and dict default parameter values behave:
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
...
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1462248344)
I actually had this as `params=[]` a few updates ago but that fails lint (see below) but I'll clean it up with a different work-around:
```
A mutable list or dict seems to be used as default parameter value:
test/functional/interface_rpc.py:41:def expect_http_rpc_status(expected_http_status, expected_rpc_error_code, node, method, params={}):
This is how mutable list and dict default parameter values behave:
>>> def f(i, j=[], k={}):
... j.append(i)
... k[i] = True
...
...
💬 darosior commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904569442)
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904569442)
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff
💬 theStack commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1904606796)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1904606796)
Concept ACK
💬 jamesob commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904681862)
ACK. Somewhat surprisingly, `decoderawtransaction` is showing a positive version number:
```
% ./src/bitcoin-cli decoderawtransaction "0000ffff01000100000000000000000000000000000000000000000000000000000000000000000000030251b2010000000100000000000000000000000000"
{
"txid": "d694a1d916d4ffa0b87dc2ed43fa8fdcf3c4a7d89afd765a8ce55ad6becea753",
"hash": "d694a1d916d4ffa0b87dc2ed43fa8fdcf3c
...
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904681862)
ACK. Somewhat surprisingly, `decoderawtransaction` is showing a positive version number:
```
% ./src/bitcoin-cli decoderawtransaction "0000ffff01000100000000000000000000000000000000000000000000000000000000000000000000030251b2010000000100000000000000000000000000"
{
"txid": "d694a1d916d4ffa0b87dc2ed43fa8fdcf3c4a7d89afd765a8ce55ad6becea753",
"hash": "d694a1d916d4ffa0b87dc2ed43fa8fdcf3c
...
💬 jb55 commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1904685588)
On Thu, Jan 18, 2024 at 02:03:35AM -0800, 0xB10C wrote:
>Hm the CI failure of `previous releases, qt5 dev package and depends
>packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four
>days ago seems weird. Didn't see any conflicts when rebasing.
>
>Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed
>the discurage -> discourage misspelling. I thought I did it in an
>earlier push.
sorry I haven't been able to sit down and actually test this yet. The
changes look g
...
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-1904685588)
On Thu, Jan 18, 2024 at 02:03:35AM -0800, 0xB10C wrote:
>Hm the CI failure of `previous releases, qt5 dev package and depends
>packages, DEBUG` (CI job used to find silent-merge conflicts (?)) four
>days ago seems weird. Didn't see any conflicts when rebasing.
>
>Anyway, rebased. Sorry for invalidating your ACK again, @jb55. Fixed
>the discurage -> discourage misspelling. I thought I did it in an
>earlier push.
sorry I haven't been able to sit down and actually test this yet. The
changes look g
...
💬 willcl-ark commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904709745)
> `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`
Do we not consider regtest "testing"?
ISTM that only "test mode" (regtest) will fail the assertion with the lines removed.
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904709745)
> `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`
Do we not consider regtest "testing"?
ISTM that only "test mode" (regtest) will fail the assertion with the lines removed.
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1462363162)
Ok I understand. I'm taking this suggestion and also using `std::move` a few places in `bitcoin-cli` where potentially large result objects are being passed. I am leaving as-is however when the argument is `NullUniValue`, I think that's correct but please let me know if I should use `std::move` at the other call sites as well.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1462363162)
Ok I understand. I'm taking this suggestion and also using `std::move` a few places in `bitcoin-cli` where potentially large result objects are being passed. I am leaving as-is however when the argument is `NullUniValue`, I think that's correct but please let me know if I should use `std::move` at the other call sites as well.
💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462341834)
line should be removed now that `supports_v2_p2p` is a property, it would probably throw an AttributeError if it was hit (which it isn't)
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462341834)
line should be removed now that `supports_v2_p2p` is a property, it would probably throw an AttributeError if it was hit (which it isn't)
📝 stickies-v opened a pull request: "rpc: improve submitpackage documentation and other improvements"
(https://github.com/bitcoin/bitcoin/pull/29292)
`submitpackage` requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.
Also sneaking in some other minor improvements that I found while going through the code:
- Informing the user that `package` needs to be an array of length between `1` and `MAX_PACKAGE_COUNT` is confusing when `IsChildWithPackage()` requires that the package size >= 2. Remove this check to avoid code duplicati
...
(https://github.com/bitcoin/bitcoin/pull/29292)
`submitpackage` requires the package to be topologically sorted with the child being the last element in the array, but this is not documented in the RPC method or the error messages.
Also sneaking in some other minor improvements that I found while going through the code:
- Informing the user that `package` needs to be an array of length between `1` and `MAX_PACKAGE_COUNT` is confusing when `IsChildWithPackage()` requires that the package size >= 2. Remove this check to avoid code duplicati
...
💬 Roasbeef commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904769620)
> ACK. Somewhat surprisingly, decoderawtransaction is showing a positive version number:
From 2019 onwards, bitcoind started to display transactions versions as unsigned everywhere: https://github.com/bitcoin/bitcoin/pull/16525
This was prompted by a `rust-bitcoin` issue pointing out they were using a `u32` instead of a `i32` for their transaction struct.
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904769620)
> ACK. Somewhat surprisingly, decoderawtransaction is showing a positive version number:
From 2019 onwards, bitcoind started to display transactions versions as unsigned everywhere: https://github.com/bitcoin/bitcoin/pull/16525
This was prompted by a `rust-bitcoin` issue pointing out they were using a `u32` instead of a `i32` for their transaction struct.
💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904776805)
> Do we not consider regtest "testing"?
The comment is vague, but it's really referring to unit tests.
When I suggested adding the two conditions allowing `pindex->nChainTx == 0` "for testing" in https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1323226418, the reason was to avoid crashes in unit tests, because some unit tests skipped calling `ReceivedBlockTransactions` and therefore skipped setting a `nChainTx` value:
https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561b
...
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904776805)
> Do we not consider regtest "testing"?
The comment is vague, but it's really referring to unit tests.
When I suggested adding the two conditions allowing `pindex->nChainTx == 0` "for testing" in https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1323226418, the reason was to avoid crashes in unit tests, because some unit tests skipped calling `ReceivedBlockTransactions` and therefore skipped setting a `nChainTx` value:
https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561b
...
💬 brunoerg commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1462427700)
```suggestion
"The package must solely consist of a child and its parents. None of the parents may depend on each other.\n"
```
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1462427700)
```suggestion
"The package must solely consist of a child and its parents. None of the parents may depend on each other.\n"
```
👋 LarryRuane's pull request is ready for review: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564)
(https://github.com/bitcoin/bitcoin/pull/26564)
💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462348934)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Nit: The documentation is in opposite order as the checks. Perhaps switch these two lines.
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462348934)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Nit: The documentation is in opposite order as the checks. Perhaps switch these two lines.
💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462350586)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
```suggestion
return strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", txid_string);
```
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462350586)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
```suggestion
return strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", txid_string);
```
💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462332681)
In the commit message of "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
> This new function takes the populated sets of
> direct and all conflicts computed in the current
> mempool, assuming the replacements are a single
> chunk, and computes a diagram check.
The first sentence is confusing to me. Could you perhaps clarify "the populated sets of direct and all conflicts" and split the sentence up?
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462332681)
In the commit message of "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
> This new function takes the populated sets of
> direct and all conflicts computed in the current
> mempool, assuming the replacements are a single
> chunk, and computes a diagram check.
The first sentence is confusing to me. Could you perhaps clarify "the populated sets of direct and all conflicts" and split the sentence up?
💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462365378)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Maybe that’s my unfamiliarity with the mempool code, but it is not obvious to me what "direct_conflicts" and "all_conflicts" are, and the description here is self-referential. Do these transactions belong to original, replacement, or both, etc.?
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462365378)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Maybe that’s my unfamiliarity with the mempool code, but it is not obvious to me what "direct_conflicts" and "all_conflicts" are, and the description here is self-referential. Do these transactions belong to original, replacement, or both, etc.?
💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462453899)
In "test: Add tests for CompareFeerateDiagram and CheckConflictTopology" (b3d415fe84be7edfbed567a79a25d406b438622b):
Nit: I found the comment here confusing. How about: "`new_diagram` is strictly better due to the first chunk, while the second chunk is worse."
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462453899)
In "test: Add tests for CompareFeerateDiagram and CheckConflictTopology" (b3d415fe84be7edfbed567a79a25d406b438622b):
Nit: I found the comment here confusing. How about: "`new_diagram` is strictly better due to the first chunk, while the second chunk is worse."