💬 jonatack commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849010201)
Oh indeed (thanks!) I think the following would be clearer.

```diff
@@ -453,12 +453,13 @@ BOOST_AUTO_TEST_CASE(getaddr_unfiltered)
addr3.nTime = Now<NodeSeconds>();
addrman->Good(addr3, /*time=*/Now<NodeSeconds>());
BOOST_CHECK(addrman->Add({addr3}, source));
- // The time is set, but after 3 unsuccessful attempts this addr should be isTerrible = true
+ // The time is set, but after ADDRMAN_RETRIES unsuccessful attempts not
+ // retried in the last minute, this
...

💬 brunoerg commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849026393)
Done, thanks.

💬 brunoerg commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#issuecomment-2486702866)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1849010201

💬 jonatack commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#issuecomment-2486751478)
re-ACK 1807df3d9fb0135057a33e01173080ea14b0403d

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2486794714)
Thank you for the review @stickies-v!

Updated 6c9121f7907262b2bf065a7ceeb8bca620060a7f -> 6c9121f7907262b2bf065a7ceeb8bca620060a7f ([kernelApi_0](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_0) -> [kernelApi_1](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_0..kernelApi_1))

* Added, cleaned up, and precised a bunch of documentation
* Slightly changed the order of a function's arguments, such that it
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849075583)
Good point, I added your suggestion.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849071009)
When I apply this to the first commit I get:
```
In file included from /home/drgrid/bitcoin/src/test/kernel/test_kernel.cpp:5:
/home/drgrid/bitcoin/src/kernel/bitcoinkernel.h:201:32: error: too many arguments provided to function-like macro invocation
201 | ) BITCOINKERNEL_ARG_NONNULL(1, 3);
| ^
/home/drgrid/bitcoin/src/kernel/bitcoinkernel.h:34:9: note: macro 'BITCOINKERNEL_ARG_NONNULL' defined here
34 | #define BITCOINKERNEL_ARG_NONNULL(_x) __at
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849072214)
Thanks, taken.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849046061)
I did not think that much about order here, but I do think having this section on the context is a good idea. The key is that the user is not required to instantiate the context for using some parts of the library (and I think this is important enough to not just make it a footnote). The user-instantiated context is only really required when interacting with the "stateful" endpoints. Besides, it may be relevant to know what the library is instantiating internally in case there is some sort of co
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849072061)
Improved this a bit and good point with the more precise language.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849053726)
I think this is good the way it is now. The options get instantiated empty and may be populated by the user. The actual object only gets configured once by the options during its instantion. It can't be changed later on, so there is no concern that users could set something at the wrong time. Having to set options as arguments in their creation function is not a clear win in my eyes either. There are use-cases, for example using the kernel only as a data reader, where the notifications are usele
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849037936)
Similarly to how the context is passed to the other chainman related functions, it is there to guarantee that it is still around when destroying the chainman. The reason for this is that there may be error notification callbacks issued during destruction.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849036598)
Thanks, I think it is good to get these little things right.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849082933)
See my [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849046061).

💬 TheCharlatan commented on pull request "Drop script_pub_key arg from createNewBlock":
(https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2486803884)
Re https://github.com/bitcoin/bitcoin/pull/31318#issuecomment-2485792072

Thank you for spelling this out @Sjors.

Concept ACK

💬 adamandrews1 commented on issue "Feature request: Backup of datadir state":
(https://github.com/bitcoin/bitcoin/issues/31324#issuecomment-2486838594)
If this goes forward, I think the backup folder path should be configurable and should have a default path outside the datadir.
Default would be disabled.
I don't see the need to keep two backup files. The backup operation should be atomic and replace the old backup file. If it fails, the old backup file should remain undisturbed.
I think API should be in terms of block height.
`-autobackup=100` would attempt to re-write the backup on new block discovery if the new block has height > 100 fro
...

👍 theStack approved a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2446762914)
re-ACK 186bc75985a0afa96eba3c5bfea09e22fe3ec2c8 🗒️

🤔 jonatack reviewed a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2446770519)
utACK c97d49628a78aac9a65f2bd1ddc733b0b425090b

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1849132153)
Sorry, I forgot to include the diff which updates the macro (and 1 instance to show it compiles):

<details>
<summary>git diff on 6c9121f790</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
index 9e6bf127db..67248349e2 100644
--- a/src/kernel/bitcoinkernel.h
+++ b/src/kernel/bitcoinkernel.h
@@ -31,9 +31,9 @@
#define BITCOINKERNEL_WARN_UNUSED_RESULT
#endif
#if !defined(BITCOINKERNEL_BUILD) && defined(__GNUC__) && BITCOINKERNEL_GNUC_PREREQ(3,
...

⚠️ pbies opened an issue: "Maximum dbcache too small"
(https://github.com/bitcoin/bitcoin/issues/31326)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

28.0 gives 'out of space' and hangs when dbcache is set to 16384 and full chainstate -reindex is run.

### Expected behaviour

Should -reindex complete the reindexing.

### Steps to reproduce

Have whole blockchain on disk, txindex=1, dbcache=16384 and reindex whole chainstate with command line -reindex option.

### Relevant log output

2024-11-18T17:15:31Z Cache size (16396074160) exceeds
...