Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 fanquake commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1486632813)
The failure here is #27316.
💬 fanquake commented on issue "wallet_create_tx.py "Not solvable pre-selected input" exception":
(https://github.com/bitcoin/bitcoin/issues/27316#issuecomment-1486633439)
Also seen here https://cirrus-ci.com/task/4836577990410240
🚀 fanquake merged a pull request: "net: #27257 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/27324)
🚀 fanquake merged a pull request: "fuzz: Remove legacy int parse fuzz tests"
(https://github.com/bitcoin/bitcoin/pull/27344)
💬 fanquake commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1486680561)
> Fuzz ones removed in https://github.com/bitcoin/bitcoin/pull/27344

Thanks. Retested at 8d31d769b7b504104f162a03c94bbc95dec7d849 and updated the op to drop those (other than in float).
💬 MarcoFalke commented on issue "-Wmaybe-uninitialized warnings under LTO":
(https://github.com/bitcoin/bitcoin/issues/27343#issuecomment-1486706789)
Yeah, that is a false positive. Not sure if they are going to fix it, but if you want to work around, you can try something like:


```diff
diff --git a/src/test/fuzz/float.cpp b/src/test/fuzz/float.cpp
index 8714ab8a04..57f8d4cec8 100644
--- a/src/test/fuzz/float.cpp
+++ b/src/test/fuzz/float.cpp
@@ -7,11 +7,13 @@
#include <test/fuzz/fuzz.h>
#include <test/fuzz/util.h>
#include <util/serfloat.h>
+#include <util/check.h>
#include <version.h>

#include <cassert>
#include <c
...
⚠️ MarcoFalke opened an issue: "wallet: Data race in GetOrCreateLegacyScriptPubKeyMan vs IsMine"
(https://github.com/bitcoin/bitcoin/issues/27354)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

tsan error

### Expected behaviour

no tsan error

### Steps to reproduce

I presume the error is intermittent, but the basic idea is that the validation interface thread (scheduler) may connect blocks and sync wallet transactions, thus call `IsMine` and access `m_spk_managers`. At the same time, the main thread may call `GetOrCreateLegacyScriptPubKeyMan`, also accessing `m_spk_managers`,
...
👍 willcl-ark approved a pull request: "init: Error if ignored bitcoin.conf file is found"
(https://github.com/bitcoin/bitcoin/pull/27302)
ACK ac9fee615

@ryanofsky Thanks for the extra clarification. I agree (and have now tested) that this PR does achieve that correctly.

I left a few tiny nits that don't need to be adressed unless you retouch (and want to address them).
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313881)
On a re-touch similar re-wording could be done here?
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150313450)
If you re-touch, perhaps this could read:

'Test error is triggered when the datadir in use contains a bitcoin.conf file that would be ignored '
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150550636)
There is `get_datadir_path()` in _test_framework/util.py_:

https://github.com/bitcoin/bitcoin/blob/b759cefe936ed3991633acff215ea1dcec5ece28/test/functional/test_framework/util.py#L417-L418

So perhaps could go next to that in a future PR?
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1150571443)
^ this can be resolved.
💬 TheCharlatan commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1486858153)
Should this get its own release note? There are bound to be some node operators that will be caught off-guard by this change.
💬 ismaelsadeeq commented on pull request "test: use address_to_scriptpubkey instead of RPC call":
(https://github.com/bitcoin/bitcoin/pull/27349#issuecomment-1486864385)
> @ismaelsadeeq seems ready for review to me
Ready for review now.
💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150513279)
nit: could use structured bindings.
```c++
for (const auto& [dest, entry] : m_address_book) {
for (const auto& [id, request] : entry.receive_requests) {
values.emplace_back(request);
}
}
```
💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150541159)
Would be good to keep the encapsulation and don't access `m_address_book` from outside of the wallet class (otherwise we are reverting #25337 and, future, #26836).
💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150538791)
This method was removed from the cpp but not from the header.
💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150523650)
No longer the case.
Same for the comment that is above this one.
💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150567795)
this also fixes abdef47f206dec114300b2d852f5c297b03179e3 (from #26644)
💬 furszy commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#discussion_r1150535358)
Not really for this PR, but.. the `used` arg is never false.

`SetAddressUsed` is only called from `SetSpentKeyState` which is only called from `AddToWallet`, which provides `used=true`.

Which.. means that if the address is not reverted to a "not used" state if the tx gets abandoned/discarded.

Could also work on it on a follow-up. Probably after #26836 so changes don't end up conflicting that much.