⚠️ ryanofsky opened an issue: "wallet_create_tx.py "Not solvable pre-selected input" exception"
(https://github.com/bitcoin/bitcoin/issues/27316)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
CI test failure https://cirrus-ci.com/task/5315879898972160?logs=functional_tests#L2675:
```
test 2023-03-23T17:23:41.645000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
self.run_test()
File "C:\Users\Con
...
(https://github.com/bitcoin/bitcoin/issues/27316)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
CI test failure https://cirrus-ci.com/task/5315879898972160?logs=functional_tests#L2675:
```
test 2023-03-23T17:23:41.645000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
self.run_test()
File "C:\Users\Con
...
💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146645081)
Thanks, you're probably right that it was intended to be covered like that so I've added your test case on `.19e-6`.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146645081)
Thanks, you're probably right that it was intended to be covered like that so I've added your test case on `.19e-6`.
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146653864)
I'll implement the ` + (bytes == 0)` for NumElemAlignBytes
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146653864)
I'll implement the ` + (bytes == 0)` for NumElemAlignBytes
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481707292)
Updated 53d99551e913bfb85769a2b34fed73898779ff0f -> 972335762aeaab557dbb2e44b149b18005987f8b ([`pr/ignoredconf.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.5) -> [`pr/ignoredconf.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.5..pr/ignoredconf.6?w=1)) to try to fix another CI error on win64:
```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\U
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481707292)
Updated 53d99551e913bfb85769a2b34fed73898779ff0f -> 972335762aeaab557dbb2e44b149b18005987f8b ([`pr/ignoredconf.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.5) -> [`pr/ignoredconf.6`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.5..pr/ignoredconf.6?w=1)) to try to fix another CI error on win64:
```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\U
...
👍 ryanofsky approved a pull request: "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it"
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test
📝 john-moffett opened a pull request: "log: Check that the timestamp string is non-empty to avoid undefined behavior"
(https://github.com/bitcoin/bitcoin/pull/27317)
Follow-up to https://github.com/bitcoin/bitcoin/pull/27233
The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior.
(https://github.com/bitcoin/bitcoin/pull/27317)
Follow-up to https://github.com/bitcoin/bitcoin/pull/27233
The current `FormatISO8601DateTime` function will return an empty string if it encounters an error when converting the `int64_t` seconds-since-epoch to a formatted date time. In the unlikely case that happens, here `strStamped.pop_back()` would be undefined behavior.
💬 MarcoFalke commented on pull request "test: getblock on header throws":
(https://github.com/bitcoin/bitcoin/pull/27237#issuecomment-1481769276)
Not sure if it makes sense to add a test only. Maybe the error message can be improved along? Closing, let's continue discussion in #https://github.com/bitcoin/bitcoin/issues/20978
(https://github.com/bitcoin/bitcoin/pull/27237#issuecomment-1481769276)
Not sure if it makes sense to add a test only. Maybe the error message can be improved along? Closing, let's continue discussion in #https://github.com/bitcoin/bitcoin/issues/20978
✅ MarcoFalke closed a pull request: "test: getblock on header throws"
(https://github.com/bitcoin/bitcoin/pull/27237)
(https://github.com/bitcoin/bitcoin/pull/27237)
💬 MarcoFalke commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146718782)
This should never happen, so instead of adding dead code, what about simply combining the two if conditions?
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146718782)
This should never happen, so instead of adding dead code, what about simply combining the two if conditions?
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1481782854)
Updated d87cb99 -> 9f947fc3d4b779f017332135323b34e8f216f613 ([pr25325.1](https://github.com/martinus/bitcoin/commits/pr25325.1) -> [pr25325.2](https://github.com/martinus/bitcoin/commits/pr25325.2))
There is a single behavior change in pool.h, now `NumElemAlignBytes` adds `+ (bytes == 0)` so that allocations of 0 bytes work with the PoolAllocator.
Other than that, updated tests to include allocation of 0 bytes, and fixed all the nits.
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1481782854)
Updated d87cb99 -> 9f947fc3d4b779f017332135323b34e8f216f613 ([pr25325.1](https://github.com/martinus/bitcoin/commits/pr25325.1) -> [pr25325.2](https://github.com/martinus/bitcoin/commits/pr25325.2))
There is a single behavior change in pool.h, now `NumElemAlignBytes` adds `+ (bytes == 0)` so that allocations of 0 bytes work with the PoolAllocator.
Other than that, updated tests to include allocation of 0 bytes, and fixed all the nits.
💬 stratospher commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481786231)
yes! thanks @mzumsande, will address this in a follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481786231)
yes! thanks @mzumsande, will address this in a follow-up PR.
💬 john-moffett commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146738163)
My thinking was that if it never happens, then this PR is unnecessary. If it could happen, then I'd rather have an explicit message rather than output that entirely lacks timestamps, which someone may not notice is bizarre.
Happy to change it, though, if people prefer your approach.
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146738163)
My thinking was that if it never happens, then this PR is unnecessary. If it could happen, then I'd rather have an explicit message rather than output that entirely lacks timestamps, which someone may not notice is bizarre.
Happy to change it, though, if people prefer your approach.
📝 furszy opened a pull request: "test: wallet_create_tx.py fix race"
(https://github.com/bitcoin/bitcoin/pull/27318)
Fixes #27316
As wallets are internally synced by the
validation interface, and the interface
dispatches events on a worker thread,
it can happen that the tx sent from the
first wallet doesn't arrive to the second
wallet before the second wallet tries to
create a new transaction using one of
the outputs of the first tx as input.
(https://github.com/bitcoin/bitcoin/pull/27318)
Fixes #27316
As wallets are internally synced by the
validation interface, and the interface
dispatches events on a worker thread,
it can happen that the tx sent from the
first wallet doesn't arrive to the second
wallet before the second wallet tries to
create a new transaction using one of
the outputs of the first tx as input.
💬 jonatack commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146755842)
Assuming it can happen, maybe wrap the message in, say, square brackets ("[...]") to be clearer in the log.
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146755842)
Assuming it can happen, maybe wrap the message in, say, square brackets ("[...]") to be clearer in the log.
💬 pinheadmz commented on issue "pruneblockchain should be able to increase the size of pruned blockchain":
(https://github.com/bitcoin/bitcoin/issues/24286#issuecomment-1481813132)
Ok, closing this for now as duplicate of https://github.com/bitcoin/bitcoin/issues/19807 which has a bit more active discussion. Will try to close that one as well in favor of #21267 if OP agrees. @prusnak please feel free to comment if you think we need to keep this one open as well.
(https://github.com/bitcoin/bitcoin/issues/24286#issuecomment-1481813132)
Ok, closing this for now as duplicate of https://github.com/bitcoin/bitcoin/issues/19807 which has a bit more active discussion. Will try to close that one as well in favor of #21267 if OP agrees. @prusnak please feel free to comment if you think we need to keep this one open as well.
✅ pinheadmz closed an issue: "pruneblockchain should be able to increase the size of pruned blockchain"
(https://github.com/bitcoin/bitcoin/issues/24286)
(https://github.com/bitcoin/bitcoin/issues/24286)
💬 john-moffett commented on pull request "log: Check that the timestamp string is non-empty to avoid undefined behavior":
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146762216)
I think I've already come around to the view that it's so unlikely as to be not worth the additional LoC, so I'll switch to the simpler version.
Thanks!
(https://github.com/bitcoin/bitcoin/pull/27317#discussion_r1146762216)
I think I've already come around to the view that it's so unlikely as to be not worth the additional LoC, so I'll switch to the simpler version.
Thanks!
💬 prusnak commented on issue "pruneblockchain should be able to increase the size of pruned blockchain":
(https://github.com/bitcoin/bitcoin/issues/24286#issuecomment-1481826196)
Ack
(https://github.com/bitcoin/bitcoin/issues/24286#issuecomment-1481826196)
Ack
💬 MarcoFalke commented on issue "tidy: enable `cppcoreguidelines-pro-type-member-init`":
(https://github.com/bitcoin/bitcoin/issues/27315#issuecomment-1481831715)
This indeed looks more involved than I initially thought. Report on the cpp files only:
```diff
diff --git a/src/.clang-tidy b/src/.clang-tidy
index b2c1b49..c5379f1 100644
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -1,5 +1,6 @@
Checks: '
-*,
+cppcoreguidelines-pro-type-member-init,
bugprone-argument-comment,
bugprone-use-after-move,
misc-unused-using-decls,
@@ -17,4 +18,3 @@ WarningsAsErrors: '*'
CheckOptions:
- key: performance-move-const-arg.CheckTriviallyCopyable
...
(https://github.com/bitcoin/bitcoin/issues/27315#issuecomment-1481831715)
This indeed looks more involved than I initially thought. Report on the cpp files only:
```diff
diff --git a/src/.clang-tidy b/src/.clang-tidy
index b2c1b49..c5379f1 100644
--- a/src/.clang-tidy
+++ b/src/.clang-tidy
@@ -1,5 +1,6 @@
Checks: '
-*,
+cppcoreguidelines-pro-type-member-init,
bugprone-argument-comment,
bugprone-use-after-move,
misc-unused-using-decls,
@@ -17,4 +18,3 @@ WarningsAsErrors: '*'
CheckOptions:
- key: performance-move-const-arg.CheckTriviallyCopyable
...
📝 brunoerg opened a pull request: "addrman, refactor: improve stochastic test in `AddSingle`"
(https://github.com/bitcoin/bitcoin/pull/27319)
In the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`). The new approach changes it to avoid it.
(https://github.com/bitcoin/bitcoin/pull/27319)
In the current implementation, if `pinfo->nRefCount` is 0, we created an unnecessary variable (`nFactor`). The new approach changes it to avoid it.