💬 maflcko commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841373951)
I'd presume it is https://github.com/corecheck/coverage-worker/blob/7d4767493be390399b54ea3cd8cafc2f068c19e2/entrypoint.sh#L58 (default flags `-O2` on Ubuntu Jammy)
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841373951)
I'd presume it is https://github.com/corecheck/coverage-worker/blob/7d4767493be390399b54ea3cd8cafc2f068c19e2/entrypoint.sh#L58 (default flags `-O2` on Ubuntu Jammy)
📝 mzumsande opened a pull request: "rpc: fix getrawtransaction segfault"
(https://github.com/bitcoin/bitcoin/pull/29003)
The crash, reported in #28986, happens when calling `getrawtransaction` for any mempool transaction with `verbosity=2`, while pruning, because the rpc calls `IsBlockPruned(const CBlockIndex* pblockindex)`, which dereferences `pblockindex` without a check.
For ease of backporting this PR fixes it just locally in `rpc/rawtransaction.cpp` by moving the check for`!blockindex` up so that the `IsBlockPruned()` will not be called with a `nullptr`. We might also want to change `IsBlockPruned()` so it
...
(https://github.com/bitcoin/bitcoin/pull/29003)
The crash, reported in #28986, happens when calling `getrawtransaction` for any mempool transaction with `verbosity=2`, while pruning, because the rpc calls `IsBlockPruned(const CBlockIndex* pblockindex)`, which dereferences `pblockindex` without a check.
For ease of backporting this PR fixes it just locally in `rpc/rawtransaction.cpp` by moving the check for`!blockindex` up so that the `IsBlockPruned()` will not be called with a `nullptr`. We might also want to change `IsBlockPruned()` so it
...
💬 mzumsande commented on issue "getrawtransaction xxxxxx.... 2 causes a segfault":
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1841379445)
See #29003 for a fix - thanks a lot for reporting, I'm really surprised that no one else has run into this before!
(https://github.com/bitcoin/bitcoin/issues/28986#issuecomment-1841379445)
See #29003 for a fix - thanks a lot for reporting, I'm really surprised that no one else has run into this before!
💬 achow101 commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1416123781)
We should avoid portraying the seed as something that is recommended for restoring a wallet. The seed is not easily exported and doing so is not a recommended method of backing up a wallet. Also not all encrypted wallets will have a seed, and not all private keys in a wallet are necessarily derived from the same seed, if derived at all.
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1416123781)
We should avoid portraying the seed as something that is recommended for restoring a wallet. The seed is not easily exported and doing so is not a recommended method of backing up a wallet. Also not all encrypted wallets will have a seed, and not all private keys in a wallet are necessarily derived from the same seed, if derived at all.
💬 aureleoules commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841400148)
> I'd presume it is [corecheck/coverage-worker@7d47674/entrypoint.sh#L58](https://github.com/corecheck/coverage-worker/blob/7d4767493be390399b54ea3cd8cafc2f068c19e2/entrypoint.sh#L58) (default flags -O2 on Ubuntu Jammy)
Yes that is correct. Also the compiler installed is `g++-11`.
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841400148)
> I'd presume it is [corecheck/coverage-worker@7d47674/entrypoint.sh#L58](https://github.com/corecheck/coverage-worker/blob/7d4767493be390399b54ea3cd8cafc2f068c19e2/entrypoint.sh#L58) (default flags -O2 on Ubuntu Jammy)
Yes that is correct. Also the compiler installed is `g++-11`.
💬 kilrau commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1841406432)
Is there another source for up-to-date full-rbf pool adoption?
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1841406432)
Is there another source for up-to-date full-rbf pool adoption?
💬 maflcko commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841418135)
lgtm ACK 494a926d05df44b60b3bc1145ad2a64acf96f61b
A test wouldn't hurt, I'd say.
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841418135)
lgtm ACK 494a926d05df44b60b3bc1145ad2a64acf96f61b
A test wouldn't hurt, I'd say.
💬 maflcko commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841426498)
I guess this was introduced in https://github.com/bitcoin/bitcoin/commit/f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841426498)
I guess this was introduced in https://github.com/bitcoin/bitcoin/commit/f86697163e5cdbc3bc4a65cfb7dbaa3d9eb602a9
💬 nogamike76 commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841429818)
No it wouldn’t. What needs to be done
May God guide us and may we all, including myself find happiness, love and
forgiveness. Be kind to someone, help someone with something that would
help them. Make sure you tel your family you love them. May the kings be
protected and may our King, Jesus Christ, always be the divine and glory,
in honor of our father, the almighty one, God.
Spirit Leader
Michael Noga jr.
Descendant of King David
Nogah, son of David
T8<3
$€£¥
On Tue, Dec 5, 20
...
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841429818)
No it wouldn’t. What needs to be done
May God guide us and may we all, including myself find happiness, love and
forgiveness. Be kind to someone, help someone with something that would
help them. Make sure you tel your family you love them. May the kings be
protected and may our King, Jesus Christ, always be the divine and glory,
in honor of our father, the almighty one, God.
Spirit Leader
Michael Noga jr.
Descendant of King David
Nogah, son of David
T8<3
$€£¥
On Tue, Dec 5, 20
...
💬 maflcko commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#discussion_r1416148547)
unrelated: I don't understand the silent fallback to verbose=1, when verbose=2 is not available. Either this should be documented, or an error should be thrown (same below).
Though, this is unrelated.
(https://github.com/bitcoin/bitcoin/pull/29003#discussion_r1416148547)
unrelated: I don't understand the silent fallback to verbose=1, when verbose=2 is not available. Either this should be documented, or an error should be thrown (same below).
Though, this is unrelated.
💬 maflcko commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841435462)
> We might also want to change `IsBlockPruned()` so it doesn't crash when called with a `nullptr`, but I didn't do that here.
I think we should avoid the use of pointers when they are assumed to not be null. In C++, references exit. However, this can be done in a follow-up.
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841435462)
> We might also want to change `IsBlockPruned()` so it doesn't crash when called with a `nullptr`, but I didn't do that here.
I think we should avoid the use of pointers when they are assumed to not be null. In C++, references exit. However, this can be done in a follow-up.
💬 aureleoules commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445584)
Here is the output of the configure script of the corecheck job if this helps:
```
1701686294337,checking for pkg-config... /usr/bin/pkg-config
1701686294338,checking pkg-config is at least version 0.9.0... yes
1701686294378,checking build system type... aarch64-unknown-linux-gnu
1701686294379,checking host system type... aarch64-unknown-linux-gnu
1701686294384,checking for a BSD-compatible install... /usr/bin/install -c
1701686294387,checking whether build environment is sane... yes
1
...
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445584)
Here is the output of the configure script of the corecheck job if this helps:
```
1701686294337,checking for pkg-config... /usr/bin/pkg-config
1701686294338,checking pkg-config is at least version 0.9.0... yes
1701686294378,checking build system type... aarch64-unknown-linux-gnu
1701686294379,checking host system type... aarch64-unknown-linux-gnu
1701686294384,checking for a BSD-compatible install... /usr/bin/install -c
1701686294387,checking whether build environment is sane... yes
1
...
💬 maflcko commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629)
Same bench result here, compiling with `clang`, both master and this pull. My command:
```
$ grep 'make_bitcoin_core=' ~/.bashrc
alias make_bitcoin_core=" ./autogen.sh && CC=clang CXX=clang++ ./configure -q --enable-c++20 --enable-experimental-util-chainstate --with-experimental-kernel-lib && make clean && make -j $(nproc)"
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1841445629)
Same bench result here, compiling with `clang`, both master and this pull. My command:
```
$ grep 'make_bitcoin_core=' ~/.bashrc
alias make_bitcoin_core=" ./autogen.sh && CC=clang CXX=clang++ ./configure -q --enable-c++20 --enable-experimental-util-chainstate --with-experimental-kernel-lib && make clean && make -j $(nproc)"
👍 pablomartin4btc approved a pull request: "rpc: fix getrawtransaction segfault"
(https://github.com/bitcoin/bitcoin/pull/29003#pullrequestreview-1765919970)
tACK 494a926d05df44b60b3bc1145ad2a64acf96f61b
I've managed to reproduce the issue, this PR fixes it.
(https://github.com/bitcoin/bitcoin/pull/29003#pullrequestreview-1765919970)
tACK 494a926d05df44b60b3bc1145ad2a64acf96f61b
I've managed to reproduce the issue, this PR fixes it.
💬 mzumsande commented on pull request "rpc: fix getrawtransaction segfault":
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841458226)
> A test wouldn't hurt, I'd say.
Will add one soon.
(https://github.com/bitcoin/bitcoin/pull/29003#issuecomment-1841458226)
> A test wouldn't hurt, I'd say.
Will add one soon.
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1841458691)
You can see full-rbf replacements and their outcome on https://mempool.space/rbf#fullrbf
On December 5, 2023 1:43:28 PM EST, Kilian ***@***.***> wrote:
>Is there another source for up-to-date full-rbf pool adoption?
>
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1841458691)
You can see full-rbf replacements and their outcome on https://mempool.space/rbf#fullrbf
On December 5, 2023 1:43:28 PM EST, Kilian ***@***.***> wrote:
>Is there another source for up-to-date full-rbf pool adoption?
>
👍 ryanofsky approved a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1765927605)
Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1765927605)
Code review ACK 406b71abcb72f234ddf9245a3f57e748343c774f. Just suggested change since last review
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1416195304)
ok cool. Updated per feedback.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1416195304)
ok cool. Updated per feedback.
💬 martinus commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1841506238)
> @martinus something similar was mentioned in [#15265 (comment)](https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636). However, with your approach every access would require 4 lookups.
Ha, of course; I didn't think about that at all.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-1841506238)
> @martinus something similar was mentioned in [#15265 (comment)](https://github.com/bitcoin/bitcoin/pull/15265#issuecomment-457720636). However, with your approach every access would require 4 lookups.
Ha, of course; I didn't think about that at all.
💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1841506898)
Updated per feedback. Thanks @andrewtoth and @naumenkogs.
Tiny diff, only renamed `m_initial_sync_finished` to `m_is_close_to_tip` in a scripted-diff commit afc29b1.
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1841506898)
Updated per feedback. Thanks @andrewtoth and @naumenkogs.
Tiny diff, only renamed `m_initial_sync_finished` to `m_is_close_to_tip` in a scripted-diff commit afc29b1.